HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Event system using callback functions in C++

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
systemcallbackusingfunctionsevent

Problem

I am learning C++ and have been trying to create an event system for use in a small game. This will be the mechanism by which game entities communicate. I would be eternally grateful if someone with experience could critique my code. Running through it myself, I'm a little concerned about the use of Runtime Type Information (RTTI). I know that this is discouraged but I can't find any alternative to providing some basic type safety.

```
class EventDispatcher
{
public:
/**
* Callback function type for event handling.
*/
template
struct Callback
{
/**
* Constructor.
*/
Callback(boost::function callback) :
handler(callback)
{
// Nothing to do.
}

/**
* Callback function.
*/
boost::function handler;
};

/**
* Constructor.
*/
EventDispatcher();

/**
* Destructor.
*/
~EventDispatcher();

/**
* Subscribe to receive events.
*
* @param callback Event handler callback.
*/
template
void Subscribe(Callback callback)
{
GetSpecificDispatcher()->Subscribe(callback);
}

/**
* Immediately broadcast an event to subscribers.
*
* @param args... Event constructor arguments.
*/
template
void Broadcast(const Arguments... args)
{
GetSpecificDispatcher()->Broadcast(EventType(args...));
}

/**
* Queue an event to be broadcast to subscribers on the next Update
* call.
*
* @param args... Event constructor arguments.
*/
template
void Enqueue(const Arguments... args)
{
GetSpecificDispatcher()->Enqueue(EventType(args...));
}

/**
* Broadcasts all events in the queue to subscribers.
*/
void Update()
{
for (auto iter = dispatchers.begin(); iter != dispatchers.end(); ++iter)
{
iter->second->Update();
}
}

protected:
/*

Solution

I'm going to update to C++20 as we go along, beginning with using modern Standard Library rather than Boost.

We need to include some headers:

#include 
#include 
#include 
#include 


EventDispatcher declares a constructor and destructor but never defines them (along with comments that add absolutely zero value):

/**
 * Constructor.
 */
EventDispatcher();

/**
 * Destructor.
 */
~EventDispatcher();


We could define them inline with =default, but I think it's better to omit them entirely and let the compiler generate implicit ones.

Conversely, ISpecificDispatcher doesn't have a destructor, but really needs one, to make it a virtual destructor, since it's used as a base class:

class ISpecificDispatcher
{
public:
    virtual ~ISpecificDispatcher() = default;
    virtual void Update() = 0;
};


Without this, delete of a base-class pointer will fail to call subclass destructors.

Now we've dealt with compiler warnings, let's look at the interface. I'd like to be able to subscribe my function:

d.Subscribe([](const int& i){ std::cout << i << '\n'; });


But the function only accepts EventDispatcher::Callback objects:

d.Subscribe(EventDispatcher::Callback{[](const int& i){ std::cout << i << '\n'; }});


We can make that a bit easier to use, with an overload of Subscribe:

template
void Subscribe(F&& callback)
{
    Subscribe(Callback{std::forward(callback)});
}


which gets us a bit closer:

d.Subscribe([](const int& i){ std::cout << i << '\n'; });


The Callback class seems too heavyweight. I simply replaced with

template
using Callback = std::function;


I don't like the construction of the event within Broadcast() and Enqueue. This forces the use of (…) rather than {…}, and doesn't seem to save anything at the call site. If we simply accept a ready-constructed EventType, like this:

template
void Enqueue(EventType&& event)
{
    GetSpecificDispatcher()->Enqueue(std::forward(event));
}


then the call site changes from

d.Enqueue(1,2);


to

d.Enqueue(event{1,2});


That's no longer, and gives the caller more options for constructing the event (which could be especially useful for factory-constructed event objects, which might not have public constructors).

There's certainly no need for SpecificDispatcher to work with anything other than EventType, since that's what its methods are always called with:

/**
     * Immediately broadcast an event to subscribers.
     */
    void Broadcast(const EventType& event)
    {
        for (auto const& callback: callbacks) {
            callback.handler(event);
        }
    }

    /**
     * Queue an event to be broadcast to subscribers on the next Update
     * call.
     */
    void Enqueue(EventType event)
    {
        eventQueue.push(std::move(event));
    }


Update() repeats the logic of Broadcast() in its inner loop - better to simply call it:

/**
     * Broadcast all events in the queue to subscribers.
     */
    virtual void Update()
    {
        while (!eventQueue.empty()) {
            auto const& event = eventQueue.front();
            Broadcast(event);
            eventQueue.pop();
        }
    }


GetSpecificDispatcher can be simpler, if we take advantage of operator[] creating a default (i.e. null pointer) object as value:

template
std::shared_ptr> GetSpecificDispatcher()
{
    auto& specificDispatcher = dispatchers[typeid(EventType).name()];
    if (!specificDispatcher) {
        specificDispatcher.reset(new SpecificDispatcher());
    }
    return std::static_pointer_cast>(specificDispatcher);
}


It's wasteful to do string comparisons here - just use typeid directly as the key type (okay, we need to wrap a reference for this):

#include 

namespace typeinfo
{
    using ref = std::reference_wrapper;

    struct hash {
        auto operator()(ref code) const {return code.get().hash_code();}
    };
    struct equality {
        auto operator()(ref lhs, ref rhs) const {return lhs.get() == rhs.get();}
    };
}


private:
    using DispatcherPointer = std::shared_ptr;

    std::unordered_map dispatchers = {};


There's no means to unsubscribe a callback when it's no longer needed. I think that subscribe() perhaps ought to return a token that can be used for subsequent removal.

Modified code

Applying these changes, we arrive at something that may be a better start for moving to compile-time event types:

```
#include
#include
#include
#include
#include

namespace typeinfo
{
using ref = std::reference_wrapper;

struct hash {
auto operator()(ref code) const
{ return code.get().hash_code(); }
};
struct equality {
auto operator()(ref lhs, ref rhs) const
{ return lhs.get() == rhs.get(); }
};
}

class EventDispatcher
{
/**
* Callback function type for event handling.
*/
template
using Callback = std::function;

public:

Code Snippets

#include <functional>
#include <map>
#include <memory>
#include <queue>
/**
 * Constructor.
 */
EventDispatcher();

/**
 * Destructor.
 */
~EventDispatcher();
class ISpecificDispatcher
{
public:
    virtual ~ISpecificDispatcher() = default;
    virtual void Update() = 0;
};
d.Subscribe([](const int& i){ std::cout << i << '\n'; });
d.Subscribe(EventDispatcher::Callback<int>{[](const int& i){ std::cout << i << '\n'; }});

Context

StackExchange Code Review Q#32586, answer score: 2

Revisions (0)

No revisions yet.