3
\$\begingroup\$

The code allows you to subscribe to events and publish events via an EventBus class. Events are identified by a source and subject. Events can have a single parameter or they can be 'indexed' and have two parameters (the first being an integer index - I will be using this a lot so wanted a specialization).

I don't particularly like the fact that:

  1. I need to have an IndexedSubscriptionmethod - it would be nicer to overload the Subscription method, but the compiler doesn't like that.
  2. The template argument needs to be specified when calling Subscription and IndexedSubscription.

I would like to know if there are any improvements I could make (I am reasonably new to C++ but am experienced in other languages).

Example use

#include <iostream>
#include <typeinfo>
#include "EventBus.h"

class Sources
{
public:
    static EventSource Source1()
    {
        static EventSource source;

        return source;
    }

    static EventSource Source2()
    {
        static EventSource source;

        return source;
    }
};

class Subjects
{
public:
    static EventSubject<double> Subject1()
    {
        static EventSubject<double> subject;

        return subject;
    }
};

int main()
{
    EventBus eb;

    eb.Subscribe<double&>(Sources::Source1(), Subjects::Subject1(), [](double& data)
    {
        std::cout << "data: " << data << std::endl;
    });

    eb.Publish<double>(Sources::Source1(), Subjects::Subject1(), 33);

    std::cout << "Done... ";
    std::getchar();

    return EXIT_SUCCESS;
}

Main event bus source

#pragma once

#include <atomic>
#include <vector>
#include <map>
#include <memory>
#include <functional>

template<class TData>
using EventHandler = std::function<void(TData)>;

template<class TData>
using IndexedEventHandler = std::function<void(int, TData)>;

template<class T>
using EventDataType = typename std::remove_reference<typename std::remove_reference<T>::type>::type;

class EventIdentifier
{
private:
    int _id;

public:
    EventIdentifier()
    {
        static std::atomic_int counter = 1;

        _id = counter++;
    }

    bool operator ==(const EventIdentifier &other) const
    {
        return _id == other._id;
    }

    bool operator<(const EventIdentifier& other) const
    {
        return _id < other._id;
    }
};

class EventSource final : public EventIdentifier
{
};

template<class TData>
class EventSubject final : public EventIdentifier
{
};

template<class TData>
class IndexedData
{
private:
    int _index;
    EventDataType<TData>& _data;

public:
    IndexedData(int index, EventDataType<TData>&& data) :
        _index(index), _data(data)
    {
    }

    int GetIndex()
    {
        return _index;
    }

    EventDataType<TData>& GetData()
    {
        return _data;
    }
};

class EventBus
{
private:
    class EventObserver
    {
    public:
        virtual void Notify(void* data) = 0;
    };

    template<class TData>
    class EventSubscription : public EventObserver
    {
    private:
        EventHandler<TData> _observer;

    public:
        EventSubscription(EventHandler<TData> observer)
        {
            _observer = observer;
        }

        virtual void Notify(void* data) override
        {
            EventDataType<TData>* pointer = static_cast<EventDataType<TData>*>(data);

            _observer(*pointer);
        }
    };

    class EventKey
    {
    private:
        EventIdentifier _source;
        EventIdentifier _subject;

    public:
        EventKey(EventIdentifier source, EventIdentifier subject) :
            _source(source), _subject(subject)
        {

        }

        bool operator<(const EventKey& other) const
        {
            if (_source < other._source)
            {
                return _subject < other._subject;
            }
            else
            {
                return false;
            }
        }
    };

    std::map<EventKey, std::vector<std::unique_ptr<EventObserver>>> _observers;

public:
    template<class TData>
    void Subscribe(EventSource source, EventSubject<EventDataType<TData>> subject, EventHandler<TData> observer)
    {
        Subscribe(EventKey(source, subject), observer);
    }

    template<class TData>
    void IndexedSubscribe(EventSource source, EventSubject<EventDataType<TData>> subject, IndexedEventHandler<TData> observer)
    {
        Subscribe<IndexedData<TData>&>(EventKey(source, subject), [observer](IndexedData<TData>& data)
        {
            observer(data.GetIndex(), data.GetData());
        });
    }

    template<class TData>
    void Publish(EventSource source, EventSubject<EventDataType<TData>> subject, TData&& data)
    {
        Publish(EventKey(source, subject), std::forward<TData>(data));
    }

    template<class TData>
    void IndexedPublish(EventSource source, EventSubject<EventDataType<TData>> subject, int index, EventDataType<TData>&& data)
    {
        Publish(EventKey(source, subject), IndexedData<TData>(index, std::forward<EventDataType<TData>>(data)));
    }

private:
    template<class TData>
    void Subscribe(EventKey key, EventHandler<TData> observer)
    {
        auto& subscripions = _observers[key];

        subscripions.push_back(std::make_unique<EventSubscription<TData>>(observer));
    }

    template<class TData>
    void Publish(EventKey key, TData&& data)
    {
        auto found = _observers.find(key);

        if (found != _observers.end())
        {
            for (auto& observer : found->second)
            {
                observer->Notify(&data);
            }
        }
    }
};
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

Missing includes

The header needs

#include <type_traits>
#include <utility>

And the demo program needs

#include <cstdio>

(or - better - just omit the final std::getchar() whose result is never used and whose error is ignored)


It's not clear why EventDataType removes references twice - C++ doesn't have references to references.

Instead of transforming types like that, it's probably better to constrain the template functions to reject reference types as arguments.


IndexedData::_data looks very dangerous, as it's a reference that's initialised from an rvalue-reference argument, so could very easily be invalidated.


I don't like laundering event data through void*, but you seem to have protected against accidents by carrying the type with the Subject class.

I think we could avoid casting, by using separate maps for subscriptions of different types.


EventBus::EventObserver has a virtual function, so we should provide it with a public virtual destructor for safety. Also, I don't think it makes sense for the Notify to modify the observer itself:

    struct EventObserver
    {
        virtual ~EventObserver() = default;
        virtual void Notify(void* data) const = 0;
    };

Consider making EventBus::EventSubscription final, which could improve the compiler's ability to optimise.


I see no way to unsubscribe from this publisher. That's problematic, particularly if we have subscriptions which have captured references.


Instead of the separate Indexed versions of functions, consider generalising to take any number of data arguments, and forwarding them as a parameter pack. That reduces code and provides more user flexibility!


The test program looks very long-winded when creating source and subject identifiers. It can be simplified:

namespace Sources
{
    const EventSource Source1;
    const EventSource Source2;
}

namespace Subjects
{
    const EventSubject<double> Subject1;
}

We needn't worry about the exact order in which these globals are initialised; any order is fine.


Consider adding names to our sources and subjects, because debugging gets hard when events are only identified by integers (and not easily predictable ones at that!).


The test program shouldn't exit with a partially-written line of output - add \n to the "Done" string.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.