4
\$\begingroup\$

I am messing around with the idea of event handling. What I tried here is to make an event handler that offers:

  • type safety
  • no need for a paired call to unregister() because it can easily be forgotten and introduce bugs (I already achived this by using weak_ptr)
  • simple usage
  • flexible (or at least not suffer of too many limitations)
  • event separation (each observer can register to a specific event)

What I try to achive further is: - event groups (the possibility to subscribe to a group of events and be notified when any of those events are triggered)

I lack the knowledge of what else could be obtained so for now I will only mention this.

The code is very rushed. I did not focus on aspects like consistent naming, const correctness, error handling, code esthetics etc. Sorry in advance for that.

struct IObserver
{
     virtual ~IObserver() = default;
};

template <typename... EventTypes>
class EventEmitter
{
protected:

    using SubscriberCallbacksPair = 
        std::pair
        <
            std::weak_ptr<IObserver>,
            std::vector 
            <
                std::variant<std::function<void(EventTypes)>...>
            >
        >;

    std::vector<SubscriberCallbacksPair> m_observers;
public:

    template<typename EventType>
    void subscribe(std::shared_ptr<IObserver> subscriber, std::function<void(EventType)> callback_function) 
    {
        auto& found = std::find_if(std::begin(m_observers), std::end(m_observers), 
            [&](const SubscriberCallbacksPair& subscriber_compare)
        {
            if (const auto& subscriber_instance = subscriber_compare.first.lock()) 
            {
                return subscriber_instance == subscriber;
            }
            return false;
        });

        if (std::end(m_observers) != found) 
        {
            auto& callbacks = std::get<1>(*found);
            callbacks.push_back(callback_function);
            return;
        }
        std::vector<std::variant<std::function<void(EventTypes)>...>> callbacks = { callback_function };

        m_observers.push_back(std::make_pair(subscriber, std::move(callbacks)));
    }

    template <typename EventType>
    void emitEvent(EventType eventObject) 
    {
        for (auto& observer : m_observers) {
            if (auto observer_instance = std::get<0>(observer).lock())
            {
                auto& callbacks = std::get<1>(observer);

                for (auto& callback : callbacks) 
                {
                    if (std::function<void(EventType)>* pointerToFunc = std::get_if<std::function<void(EventType)>>(&callback))
                    {
                        (*pointerToFunc)(eventObject);
                    }
                }
            }
        }
    }
};

An event is a simple structure that contains data:

struct MouseMovedEvent
{
    int x;
    int y;
};

An example of an EventEmitter

struct MouseEventEmitter : EventEmitter
    <
    MouseMovedEvent,
    KeyBoardEvent
    >
{

    void moveMouse()
    {
        MouseMovedEvent mouseEvent;
        mouseEvent.x = 2;
        mouseEvent.y = 3;

        this->emitEvent(mouseEvent);
    }
};

And below an observer. There are no constraints other than inheriting from the IObserver interface.

struct MouseMovedObserver1 : IObserver
{
    void OnMouseMoved(int x, int y) 
    {
        std::cout << x << " " << y << "\n";
    }
};

Putting everything together:

    MouseEventEmitter eventEmitter;

    auto observer = std::make_shared<MouseMovedObserver1>();

    std::function<void(MouseMovedEvent)> obsFuncMouseMove
        = [&](const MouseMovedEvent& mouseEvent) { observer->OnMouseMoved(mouseEvent.x, mouseEvent.y); };

    eventEmitter.subscribe(observer, obsFuncMouseMove);

    eventEmitter.moveMouse();

What I wish to know if there are any concerns someone should have if it was to use this code in a project (ex: Maybe there are some performence issues I should be careful about or maybe there are some usage limitations I could not see by just typing a simple example)

\$\endgroup\$
3
\$\begingroup\$

I'm actually not the right person to properly review your code, since event handling is outside my particular experience, so the overall idea of even having this kind of observer-handler-emitter system might be great or might be a bad idea, I'm not judging. Still, in the context of what you're trying to do, here are several comments.

Don't skimp on type definitions

You're missing

using Callback = std::variant<std::function<void(EventTypes)>...>

so you're repeating yourself with that long type definition in a couple of places (and it's not that readable).

The IObserver class - an ugly artifact

The IObserver class has no functionality implemented - not even virtually. You seem to be using it merely for its virtual destructor. You should probably avoid defining this kind of a class.

Also, I'm personally against Hungarian-notation-like naming, such as prending I, but that's a stylistic preference.

The observer design - a job half-done

You've made it a point that you want to avoid having to explicitly unsubscribe() event handlers, and for that purpose you're willing to equate the end-of-lifetime of an observer with its unsubscription time. Well, then, just go all the way on the other side: Avoid subscribe() by having the construction of an observer do the subscription. By doing that, you will gain many things:

  • No need to explicitly subscribe() - constructing an observer (which gets passed an emitter) automatically subscribes all relevant handlers.
  • No need for the emitter to keep a data structure of callbacks per observer - just a set of event types to which it is subscribed.
  • Actually having some functionality in the Observer class
  • With some clever use of templates and constexpr code you might just be able to avoid writing any code in specific observers other than the actual event handlers, and just have them inherit from Observer<EventType1, EventType2, EventType3> for example - with templated code in the Observer class taking care of everything, both on construction and on event emission (the emitter will call a method of Observer after all.

Not thread-safe

Event-related code is typically used in a concurrent/multi-threaded environment. Your code does not seem to be thread-safe (e.g. possible race condition appending to m_observers).

Sequential execution of handlers

You're calling the handlers one at a time. Now, perhaps that's reasonable - if you can assume they all take very little time. But if that's not the case, you might consider launching them "all at once", i.e. in their own threads on which you join, or using some executor mechanism etc.

misnamed m_observers

your m_observers holds callbacks mainly, or callback and weak pointer to observers. Its name suggests otherwise.

Avoid using std::pair

std::pair considered harmful!

Define a proper struct, with named members, and make the struct and member names meaningful. A method or two might not be a bad idea even if it's essentially a plain-old-data struct, if that increases readability.

In your case, however, this is a moot point, due to:

Inappropriate data structure for holding callbacks

The callbacks don't have any particular order. (They could have, if that's important to you; right now it's order of insertion, which is not very meaningful.) So - why are you keeping them in an std::vector? It's not as though you need killer performance and an std::unordered_set is too much overhead, right? Also, even if it was, and you wanted an std::vector - encapsulate that away in some kind of vector-based-set class (even if merely via a type definition).

... but actually, a set won't do either. After all, callbacks are per-observer. So it's a set of pairs. And what's a set of pairs? That's right, a(n unordered) map. So what you should really have there is:

std::unordered_map<std::weak_ptr<IObserver>,std::unordered_set<Callback>> m_callbacks;

(before any additional typedef'ing you might do.)

\$\endgroup\$
  • \$\begingroup\$ Thank you very much for the time and effort you have put in your answer! I agree with your observations and I will modify the code accordingly. There is just one more thing I want to ask you regarding the sequential execution of handlers. Can you tell me more about this executor mechanism and about launch the callback in separate threads ? Should I make a thread pool and launch in callbacks in one of the free threads ? How would you design such a thing ? \$\endgroup\$ – Catalin Manole Sep 11 '17 at 8:12
  • \$\begingroup\$ @CatalinManole: Just don't modify the code here. We leave code we post the way we posted it so as not to invalidate the reviews. \$\endgroup\$ – einpoklum Sep 11 '17 at 8:14
  • \$\begingroup\$ @CatalinManole: To answer the actual question: Again, don't reinvent the wheel. Plenty of executor and thread pool libraries in C++ out there, e.g. this one. \$\endgroup\$ – einpoklum Sep 11 '17 at 8:19
  • \$\begingroup\$ I won't modify the code posted here, don't worry. The reason I asked was because I try to implement it myself so I can have a better insight of what is happening inside such a construct. Thank you once again for the feedback! \$\endgroup\$ – Catalin Manole Sep 11 '17 at 8:25
  • \$\begingroup\$ FWIW I prefix for interface types is deeply anchored and much more of a strongly established convention than a stylistic preference. In this case I'm not sure it is an interface though (given it's a struct - although, I don't do C++ so don't hold me to it). \$\endgroup\$ – Mathieu Guindon Sep 11 '17 at 14:02

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.