1
\$\begingroup\$

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

\$\endgroup\$
4
\$\begingroup\$

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);
}
\$\endgroup\$
3
  • \$\begingroup\$ The EVET_TYPE_FUNC macro defines a member function to return type as string, it's purpose is to for logging \$\endgroup\$ – kdcode May 4 '16 at 10:41
  • \$\begingroup\$ The EventListenerHandle has it's own remove function to let the user remove a listener without knowing the original event type which the listener was registered with. \$\endgroup\$ – kdcode May 4 '16 at 10:46
  • \$\begingroup\$ The KDFA_DEFINE_VOID_EVENT macro generates an struct with no member but the eventType() function, which is an event with no extra parameter, It's a mere convenience macro \$\endgroup\$ – kdcode May 4 '16 at 10:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.