C++ simple event system

Question

I posted an earlier version of this code a couple of days ago, so it's the updated version of the code posted here.

#include <iostream>
#include <functional>
#include <conio.h>
#include <unordered_map>
#include <string>

class EventListenerHandle
{
        template <typename U>
        friend class EventChannel;
    private:
        std::function<void()> removeFunction;
        bool removed;
        EventListenerHandle() = delete;
        inline EventListenerHandle(std::function<void()> remove_function) :removed(false)
        {
            this->removeFunction = remove_function;
        }
    public:
        inline void remove()
        {
            this->removed = true;
            removeFunction();
        }
};

template <typename T>
class EventChannel
{
private:
    using idType = unsigned int;
    static const idType& getNextId()
    {
        static idType id = 0;
        return id++;
    }

    static std::unordered_map<idType, std::function<bool(const T&)>>& getHandlers()
    {
        static std::unordered_map<idType, std::function<bool(const T&)>> handlers;
        return handlers;
    }
    static void removeListener(idType listener_id)
    {
        getHandlers().erase(listener_id);
    }
public:
    static EventListenerHandle registerListener(std::function<bool(const T&)> listener)
    {
        auto& id = getNextId();
        getHandlers()[id] = listener;
        return EventListenerHandle(std::bind(EventChannel<T>::removeListener, id));

    }
    static void tringgerEvent(T e)
    {
        for (const auto& h : getHandlers())
        {
            h.second(e);
        }
    }

};
/*to force the caller to  pass the bare type (not pointer or reference) as the templapte parameter
if these where not here one could (and possibly would) write some code like:
EventChannel<TouchEvent&>::registerListener([](const TouchEvent& ev)->bool
{
});
TouchEvent t;
DISPATCH_EVENT(t);
and expect the listener to be called. so it's better to prevent them from
using pointer and reference types as template parameter for EventChannel class.
also i decided not to care for volatile types.

*/

template<typename T>
class EventChannel<T*>{};

template<typename T>
class EventChannel<T&>{};

template <typename T>
class EventChannel<const T>{};

//just for convenience
template <typename T>
void DISPATCH_EVENT(const T& t)
{
    EventChannel<T>::tringgerEvent(t);
    //std::cout << t.eventType();
}
//to let the caller pass pointers as the event object
template <typename T>
void DISPATCH_EVENT(T* t)
{
    DISPATCH_EVENT<T>(*t);
}
#define EVET_TYPE_FUNC(T) static std::string eventType(){return #T;}
#define KDFA_DEFINE_VOID_EVENT(__NAME__)    struct __NAME__{EVET_TYPE_FUNC(__NAME__)};
struct TouchEvent
{
    int t_id;
    float x;
    float y;
    EVET_TYPE_FUNC(TouchEvent);
};



KDFA_DEFINE_VOID_EVENT(CAMERA_MOVED_OR_ZOOMED);
void main()
{
    auto lid = EventChannel<TouchEvent>::registerListener([](const TouchEvent& ev)->bool
    {
        std::cout << "got touch " << ev.t_id << "\n";
        return false;
    });



    TouchEvent t;
    t.t_id = 10;

    DISPATCH_EVENT(t);//dispatch with object
    DISPATCH_EVENT(&t);// dispatch with pointer

    //it's best to use RAII to correctly call remove on event listener handles
    lid.remove();//remove listener
    lid.remove();//calling remove multiple times would not cause any problems. this is a mere design decision


    DISPATCH_EVENT(t);



    _getch();
}

consider reading the main function first, it's pretty simple to understand. As I lost the ability to think about this design out of the box, any comment is welcomed. Thanks

Answer 1

Fix getNextId

static const idType& getNextId()
{
   static idType id = 0;
   return id++;
}

is not right.

1. There is no need to return a const idType&. It can be just idType.

2. If you want to return a const idType&, you need to change the return statement to:

   return ++id;
   

   Using

   return id++;
   

   has couple of problems.

   • The value of the first ID will be 0, not 1. Is that your intention?

   • id++ is an rvalue, not an lvalue. It is like:

     int temp = id;
     ++id;
     return temp;
     

   I get the following compiler warning with your code using g++ 4.8.4:

    warning: returning reference to temporary [-Wreturn-local-addr]
     return id++;
   

In conclusion, I suggest:

static idType getNextId()
{
   static idType id = 0;
   return ++id;
}

Fix return type of main

You have

void main() { ... }

The standard requires the return type of main to be int. Change it to:

int main() { ... }

Change physical layout of the classes

The public section of a class are most important to users. Hence, I recommend the pubilc section listed first. If we could, we could completely hide the private section from the users. Hence, I recommend the private section to be listed last.

class EventListenerHandle
{
   public:

      // Add the public members

   private:

      // Add the public members
};

Of course, I recommend this change for Channel too.

Unused macro

You have

#define KDFA_DEFINE_VOID_EVENT(__NAME__)    struct __NAME__{EVET_TYPE_FUNC(__NAME__)};

It's not used anywhere in your posted code. It's not clear what the purpose of the macro is.

Unclear purpose of macro

You also have

#define EVET_TYPE_FUNC(T) static std::string eventType(){return #T;}

which is used in:

struct TouchEvent
{
   int t_id;
   float x;
   float y;
   EVET_TYPE_FUNC(TouchEvent);
};

However, removing the line

   EVET_TYPE_FUNC(TouchEvent);

from TouchEvent does not create any compiler or linker errors. It's not clear what the purpose of creating eventType() is.

Don't use ALL CAPS for functions

You have

template <typename T>
void DISPATCH_EVENT(const T& t)
{
   EventChannel<T>::tringgerEvent(t);
}

template <typename T>
void DISPATCH_EVENT(T* t)
{
   DISPATCH_EVENT<T>(*t);
}

All caps are usually reserved for macros, not for functions. Change them to:

template <typename T>
void dispatchEvent(const T& t)
{
   EventChannel<T>::tringgerEvent(t);
}

template <typename T>
void dispatchEvent(T* t)
{
   // No need for explicit use of T here.
   // That should be correctly deduced.
   // dispatchEvent<T>(*t);
   dispatchEvent(*t);
}

Registering and removing listeners is not symmetric

You have:

EventChannel<T>::registerListener() { ... }

for registering listeners but removing listeners is accomplished using

EventListenerHandle::remove()

I recommend adding a function removeListener in EventChannel.

EventChannel<T>::removeListener(EventListenerHandle handle) { ... }

If you do that, the only member data needed in EventListenerHandle will be just the ID of the event.

Suggested event handling code

#include <iostream>
#include <functional>
#include <unordered_map>
#include <string>

struct EventListenerHandle
{
   unsigned int id;
};

template <typename T>
class EventChannel
{
   public:

      using idType = unsigned int;

      static EventListenerHandle registerListener(std::function<bool(const T&)> listener)
      {
         auto id = getNextId();
         getHandlers()[id] = listener;
         return EventListenerHandle{id};    
      }

      static void removeListener(EventListenerHandle handle)
      {
         getHandlers().erase(handle.id);
      }

      static void tringgerEvent(T e)
      {
         for (const auto& h : getHandlers())
         {
            h.second(e);
         }
      }

   private:
      static idType getNextId()
      {
         static idType id = 0;
         return ++id;
      }

      static std::unordered_map<idType, std::function<bool(const T&)>>& getHandlers()
      {
         static std::unordered_map<idType, std::function<bool(const T&)>> handlers;
         return handlers;
      }
};

template<typename T>
class EventChannel<T*>{};

template<typename T>
class EventChannel<T&>{};

template <typename T>
class EventChannel<const T>{};

template <typename T>
void dispatchEvent(const T& t)
{
   EventChannel<T>::tringgerEvent(t);
}

template <typename T>
void dispatchEvent(T* t)
{
   dispatchEvent(*t);
}

Test code

struct TouchEvent
{
   int t_id;
   float x;
   float y;
};

int main()
{
   auto lid = EventChannel<TouchEvent>::registerListener([](const TouchEvent& ev)->bool
                                                         {
                                                         std::cout << "got touch " << ev.t_id << "\n";
                                                         return false;
                                                         });

   TouchEvent t;
   t.t_id = 10;

   dispatchEvent(t);  // dispatch with object
   dispatchEvent(&t); // dispatch with pointer

   EventChannel<TouchEvent>::removeListener(lid);

   dispatchEvent(t);
}