1
\$\begingroup\$

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);
\$\endgroup\$
1
  • \$\begingroup\$ Welcome to CodeReview@SE. The indentation of the code presented looks inconsistent; two pieces of advice: 1) use blanks, only 2) bracket each code block with lines containing just ~~~ (and possibly a language hint) instead of prefixing each line with another 4 blanks. \$\endgroup\$
    – greybeard
    Jan 21 '20 at 22:25
4
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your reply! I will try to implement what you said, hopefully I will end up with something decent. And when I said that I was making a simple event system I ment that the code was primitive, maybee I should have said basic. \$\endgroup\$ Jan 22 '20 at 18:18
  • \$\begingroup\$ Basic and primitive is sometimes what you want. A teapot is primitive in that it doesn't have many moving or complex parts, but it's intuitive and performs its job very well. \$\endgroup\$
    – butt
    Jan 22 '20 at 18:25

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.