Event Dispatching System

Question

Below you can see my implementation of a simple event dispatcher. I would like to get some feedback or improvement tips regarding the design and maybe an idea how to get rid of the static_cast. Thank you very much.

Classes

Event.h

#pragma once

#include <cstdint>

enum class EventType : uint8_t {
    ButtonPress,
    FactoryReset
};

class Event {
    public:
        Event() = default;
        explicit Event(EventType type) : mType(type) {}
        virtual ~Event() = default;

        inline const EventType Type() const { return this->mType; };

    protected:
        EventType mType;
};

ButtonPressEvent.h

#pragma once

#include "Event.h"

class ButtonPressEvent final : public Event {
    public:
        ButtonPressEvent(uint8_t pin) : Event(EventType::ButtonPress), mPin(pin) {};

        uint8_t mPin;
};

FactoryResetEvent.h

#pragma once

#include "Event.h"

class FactoryResetEvent final : public Event {
    public:
        FactoryResetEvent() : Event(EventType::FactoryReset) {};
};

Subscriber.h

#pragma once

#include <functional>
#include <map>
#include <memory>

#include "Event.h"

using EventHandlerType = std::function<void(std::unique_ptr<Event>&)>;

class Subscriber {
    public:
        virtual ~Subscriber() = default;
        virtual std::map<EventType, EventHandlerType> GetSubscribedEvents() = 0; 
};

Dispatcher.h

#pragma once

#include <map>
#include <memory>
#include <vector>

#include "Event.h"
#include "Subscriber.h"

class Dispatcher {
    public:
        Dispatcher();

        void Listen(EventType type, const EventHandlerType &funct);
        void Subscribe(Subscriber *subscriber);
        void Post(std::unique_ptr<Event> &ptr);
        void Process();

    private:
        std::map<EventType, std::vector<EventHandlerType>> mObservers;
        std::vector<std::unique_ptr<Event>> mEvents;
};

Dispatcher.cpp

#include "Dispatcher.h"

void Dispatcher::Listen(EventType type, const EventHandlerType &funct) {
    this->mObservers[type].push_back(funct);
}

void Dispatcher::Subscribe(Subscriber *subscriber) {
    for (auto&& event : subscriber->GetSubscribedEvents()) {
        this->Listen(event.first, event.second);
    }
}

void Dispatcher::Post(std::unique_ptr<Event> &ptr) {
    this->mEvents.push_back(std::move(ptr));
}

void Dispatcher::Process() {
    if (this->mEvents.empty()) {
        return;
    }

    for (auto&& event : this->mEvents) {
        // call listener functions
        for (auto&& observer : this->mObservers.at(event->Type())) {
            observer(event);
        }
    }

    this->mEvents.clear();
}

Usage

App.h

#pragma once

#include "Subscriber.h"

class App : public Subscriber {
    public:
        std::map<EventType, EventHandlerType> GetSubscribedEvents();
        void OnButtonPressEvent(std::unique_ptr<Event> &event);
};

App.cpp

#include <functional>
#include <stdio.h>

#include "App.h"
#include "Event.h"
#include "ButtonPressEvent.h"

std::map<EventType, EventHandlerType> App::GetSubscribedEvents() {
    return {
        { EventType::ButtonPress, std::bind(&App::OnButtonPressEvent, this, std::placeholders::_1) }
    };
}

void App::OnButtonPressEvent(std::unique_ptr<Event> &event) {
    ButtonPressEvent *b = static_cast<ButtonPressEvent*>(event.get());

    printf("Button pin %d pressed", b->mPin);
}

main.cpp

#include "Dispatcher.h"
#include "ButtonPressEvent.h"
#include "App.h"

void main(void) {
    App app;
    Dispatcher dispatcher;

    dispatcher.Subscribe(&app);

    // simulate event
    std::unique_ptr<Event> ptr(new ButtonPressEvent(11));
    dispatcher.Post(ptr);

    while (true) {
        dispatcher.Process();
    }
}

Answer 1

Delete the default constructor of Event

The default constructor of Event doesn't initialize mType, leaving it in an undefined state. You probably never want to create a default initialized Event object, so it would be best to delete it to catch any incorrect use:

Event() = delete;

Make mType const

Assuming you never want to change the type of an event after creating it, you should make the member variable mType const.

Use std::unordered_map instead of std::map where possible

Since you don't require mObservers to be ordered in any way, use std::unordered_map, as it has \$O(1)\$ lookups instead of \$O(\log N)\$.

Consider the overhead of returning a std::map

GetSubscribedEvents() is a very costly operation, since it returns a std::map by value. In this case, it is more efficient to let the subscriber call Listen() instead of the dispatcher. So instead of GetSubscribedEvents(), you could instead have a member function Subscribe() like so:

void App::Subscribe(Dispatcher& dispatcher) {
    dispatcher.Listen(EventType::ButtonPress,
                      std::bind(&App::OnButtonPressEvent, this, std::placeholders::_1));
}

And instead of:

dispatcher.Subscribe(&app);

Call:

app.Subscribe(dispatcher);

Lifetime management of events

There are several issues with how you manage the lifetime of Event objects. It is a good idea to use std::unique_ptr, as this avoids memory management issues. However, you still need to consider how and when ownership is transferred. The function Post() takes a reference to a std::unique_ptr<Event>, and then internally std::move()s it into the vector mEvents. While this is legal, it is hard to see just from the API that Post() is taking over ownership. It is better to make this explicit, by having Post() take a forwarding reference:

void Dispatcher::Post(std::unique_ptr<Event>&& ptr) {
    mEvents.push_back(std::move(ptr));
}

This now forces the caller to use std::move() as well, which makes it clear at the call site that ownership is transferred:

std::unique_ptr<Event> ptr(new ButtonPressEvent(11));
dispatcher.Post(std::move(ptr));

While it looks like more work, it now actually supports passing in a temporary variable, like so:

dispatcher.Post(std::make_unique<ButtonPressEvent>(11));

In fact, since it is likely that you want to create and post the event in one go, you could make Post() a template that does this:

template<typename T, typename... Args>
void Post(Args&&... args) {
    mEvents.push_back(std::make_unique<T>(std::forward<Args>(args)...));
}

And then the caller can do:

dispatcher.Post<ButtonPressEvent>(11);

Another lifetime issue happens when the observers are called. Again, you pass the event as a reference to a std::unique_ptr<Event>, since you defined the type of an observer as:

using EventHandlerType = std::function<void(std::unique_ptr<Event>&)>;

However, this allows an observer to modify the event, and even std::move() the event out of the std::unique_ptr. This is clearly very undesirable, as there might be multiple observers, and they all should see the same event. Instead, you should pass a const reference to the Event:

using EventHandlerType = std::function<void(const Event&)>;

Call the observers like so:

for (auto& event : mEvents)
    for (auto& observer : mObservers.at(event->Type()))
        observer(*event);

And then the observer looks like:

void App::OnButtonPressEvent(const Event& event) {
    auto& b = static_cast<const ButtonPressEvent&>(event);

    std::cout << "Button pin " << b.mPin << " pressed\n";
}

Answer 2

Be consistent with the coding, currently there are differt strategies for creating default constructors in use.

Prefer to include Header Files in cpp source files

By including header files in header files the code may become a maintenance problem and the size of the code that is compiled after the preprocessor runs may be unnecessary bloated. People who are maintaining code may not look at the header files when they are altering C++ source files. This may also reduce the number of files that need to compile when a header file changes.

The this Object

Generally in C++ one does not need to use the this object to point to member variables or methods. The members can be accessed just by name. In Dispatcer.cpp there is no need to use this. It might have been necessary in the compiler you were using because the constructor wasn't defined. In the header file FactorResetEvent.h The constructor could be a default constructor as well.

#include "Dispatcher.h"

void Dispatcher::Listen(EventType type, const EventHandlerType& funct) {
    mObservers[type].push_back(funct);
}

void Dispatcher::Subscribe(Subscriber* subscriber) {
    for (auto&& event : subscriber->GetSubscribedEvents()) {
        Listen(event.first, event.second);
    }
}

void Dispatcher::Post(std::unique_ptr<Event>& ptr) {
    mEvents.push_back(std::move(ptr));
}

void Dispatcher::Process() {
    if (mEvents.empty()) {
        return;
    }

    for (auto&& event : mEvents) {
        // call listener functions
        for (auto&& observer : this->mObservers.at(event->Type())) {
            observer(event);
        }
    }

    mEvents.clear();
}

Default Constructors

If you are not going to create a constructor for an object, such as the Dispatcher the constructor can be defined as a default constructor. In Subscriber.h the default destructor is used, but there is not constructor defined at all.

class Dispatcher {
public:
    Dispatcher() = default;

    void Listen(EventType type, const EventHandlerType& funct);
    void Subscribe(Subscriber* subscriber);
    void Post(std::unique_ptr<Event>& ptr);
    void Process();

private:
    std::map<EventType, std::vector<EventHandlerType>> mObservers;
    std::vector<std::unique_ptr<Event>> mEvents;
};

My compiler and linker (Visual Studio 2019) reported that the constructor was undefined when I compiled the program.