Creating a simple event system

Question

I am trying to make a very simple event system for learning purposes, after seeing some much more complicated code on the internet I was wondering if what I am doing is fine of if it has major issues. The code works but I am not sure if it is fast and safe.

The part that worries me is that in my test Listener class (KeyListener) I have to do a cast in order to get the correct Event, is this considered bad practice?

Thanks for your help!

Here is the code:

Event.h

      extern unsigned int NULL_EVENT;

      extern unsigned int KEY_PRESSED_EVENT;

      extern unsigned int EVENT_AMOUNT;

      class Event
      {
      public:
        Event();
        Event(const Event&);
        Event& operator=(const Event&);

        virtual unsigned int getType() const;
      };

Listener.h:

      class Listener
      {
      public:
        Listener();
        Listener(const Listener&);
        Listener& operator=(const Listener&);

        virtual unsigned int getEventType() const;
        virtual void onEvent(Event*);
      };

EventHandler.h:

      class EventHandler
      {
      private:
        LinkedList<Listener*>* listeners;

        void deleteData();
      public:
        EventHandler();
        EventHandler(const EventHandler&);
        EventHandler& operator=(const EventHandler&);
        ~EventHandler();

        void registerListener(Listener*);
        void onEvent(Event&);
      };

EventHandler.cpp:

EventHandler::EventHandler()
      {
        listeners = new LinkedList<Listener*>[EVENT_AMOUNT];
      }

      EventHandler::EventHandler(const EventHandler& handler)
      {
        listeners = new LinkedList<Listener*>[EVENT_AMOUNT];
        for(unsigned int i=0;i<EVENT_AMOUNT;i++)
        {
          *(listeners + i) = *(handler.listeners + i);
        }
      }

      EventHandler& EventHandler::operator=(const EventHandler& handler)
      {
        deleteData();
        listeners = new LinkedList<Listener*>[EVENT_AMOUNT];
        for(unsigned int i=0;i<EVENT_AMOUNT;i++)
        {
          *(listeners + i) = *(handler.listeners + i);
        }
        return *this;
      }

      EventHandler::~EventHandler()
      {
        deleteData();
      }

      void EventHandler::deleteData()
      {
        delete[] listeners;
      }

      void EventHandler::registerListener(Listener* listener)
      {
        unsigned int id = listener->getEventType();
        assert(id < EVENT_AMOUNT && "Event type is out of bounds!");
        if(!(listeners + id)->contains(listener))
        {
          std::cout << "registering listener of id: " << id << std::endl;
          (listeners + id)->add(listener);
        }
      }

      void EventHandler::onEvent(Event& event)
      {
        unsigned int id = event.getType();
        assert(id < EVENT_AMOUNT && "Event type is out of bounds!");
        LinkedListIterator<Listener*> itt = (listeners + id)->getIterator();
        for(;itt.isValid();itt.next())
        {
          itt.getElement()->onEvent(&event);
        }
      }

Here is test Event and a test Listener: KeyPressedEvent.h:

class KeyPressedEvent : public Event
{
protected:
  unsigned int keyCode;

public:
  KeyPressedEvent(unsigned int);
  KeyPressedEvent(const KeyPressedEvent&);
  KeyPressedEvent& operator=(const KeyPressedEvent&);

  unsigned int getKeyCode() const;

  virtual unsigned int getType() const override;
};

KeyPressedEvent.cpp

KeyPressedEvent::KeyPressedEvent(unsigned int keyCode)
        {
          this->keyCode = keyCode;
        }

        KeyPressedEvent::KeyPressedEvent(const KeyPressedEvent& event)
        {
          keyCode = event.keyCode;
        }

        KeyPressedEvent& KeyPressedEvent::operator=(const KeyPressedEvent& event)
        {
          keyCode = event.keyCode;
          return *this;
        }

        unsigned int KeyPressedEvent::getKeyCode() const
        {
          return keyCode;
        }

        unsigned int KeyPressedEvent::getType() const
        {
          return KEY_PRESSED_EVENT;
        }

KeyListener.h:

class KeyListener : public Listener
{
public:
  KeyListener();

  virtual unsigned int getEventType() const override;
  virtual void onEvent(Event*) override;
};

KeyListener.cpp:

KeyListener::KeyListener()
{

}

unsigned int KeyListener::getEventType() const
{
  return KEY_PRESSED_EVENT;
}

void KeyListener::onEvent(Event* e)
{
  KeyPressedEvent* event = (KeyPressedEvent*)e; //Even though I am sure that this is the correct event type, can I do things like this or is this considered bad practice?
  std::cout << "key pressed: " << event->getKeyCode() << std::endl;
}

main.cpp:

EventHandler handler;
  KeyListener listener;

  handler.registerListener(&listener);

  KeyPressedEvent event(5);
  handler.onEvent(event);

Answer 1

I am trying to make a very simple event system for learning purposes,

You've actually made a very complex event system.

First, let's explore the requirements of an "event system". I am basing these off a rough examination of your code.

• Custom Events can be defined
• Handlers register Listeners
• Calls to Handler::onEvent get dispatched to all registered Listeners

A system which satisfies these requirements could be implemented with a std::vector of std::functions. Instead of a Listener class, it would just be a field or perhaps a type alias. The event structure could be specified in the std::function<> type.

Let's use this concept as a reference.

---

Event.h

You are declaring every event type in this header essentially as a global variable. This means:

• Code can change the event values at runtime
• You need to modify at least two different files just to declare a new event type
• Events values may be important. Are two different types an alias of each other or not? You won't know until runtime.
• Every new event type created will trigger a rebuild of every dependency of Event.h
• Different runtime modules may disagree on the values of the event types due to this linkage

The event class serves only to create a common base class for other event types. However, I don't see any utility for this. In the straw example I described, handlers must be coupled to their event types to some degree, but not to any base class. The implementation here requires every client of the system to be coupled to every other client.

Prefer to use enums, and declare them as privately as possible.

---

LinkedList appears to be a learning-exercise utility. As a general rule, prefer std::vector when storing a collection of objects. They're very fast and canonically ubiquitous. Note that the algorithmic complexity of adding or removing from the collection is not a good reason by itself to use a linked list over a std::vector. In this case, the collection stores pointers which are very small, so there's a lot of overhead in each linked list node. Also, every time a linked list node is iterated over, it may induce a cache miss due to indirecting, which a std::vector is much less likely to do.

---

As a learning exercise, I recommend trying to trim away at your code until it is at the bare bones of functionality. Do you really need a default constructor that does nothing? Do you need a copy constructor on a type with no data? These should be questions raised when trying to slim down your system.