6
\$\begingroup\$

I'm trying to create a modern template class for a generic observable:

typedef int SUBSCRIPTION_ID;

template<typename EventType, typename Event>
    class Subject2
    {
        typedef struct
        {
            std::function<void(const Event&)> method;
            int id;
        } Callable2;

    public:
        virtual SUBSCRIPTION_ID subscribe(const EventType& event, std::function<void(const Event&)>&& call)
        {
            Callable2 this_call = { call, ++last_id_ };
            observers_[event].push_back(this_call);
            return this_call.id;
        }

        virtual void unsubscribe(const EventType& event, SUBSCRIPTION_ID subscription)
        {
            std::vector<Callable2>& subscribers = observers_[event];
            for (auto& obs = subscribers.begin(); obs != subscribers.end(); ++obs) {
                if ((*obs).id == subscription) {
                    subscribers.erase(obs);
                    return;
                }
            }
            assert(false);
            return;
        }

    protected:
        void fire(const Event& data)
        {
            auto& subscribers = observers_[data.event];
            for (const auto& obs : subscribers) {
                obs.method(data);
            }
        }

    protected:
        std::map<EventType, std::vector<Callable2>> observers_;
        int last_id_ = 0;
    };

(obviously Subject2 and Callable2 are in fact Subject and Callable)

A simple example of how I use it is:

something->subscribe(MouseEvent::Type::MOVE, [&](const MouseEvent& evt) { 
    do_something_usefull();
});

The code runs quite ok, however there are a couple of things that I'm sure they could be improved but am unsure how:

First is the technique I use for unsubscribing, as the code shows I create an ID and store it within the Callable2 struct that is pushed into the vector. Ideally this could be done without the need for such artifact and extra level of indirection but then, how?

Second, I don't really understand why the Callable2 structure can't be written as to store a reference rather than a function object, tried unsuccessfully with:

    typedef struct
    {
        std::function<void(const Event&)>& method;
        int id;
    } Callable2;

Last but not least, another thing I was trying was to simplify the template from

template<typename EventType, typename Event>

To

template<typename Event>

but then it won't compile if I try:

virtual SUBSCRIPTION_ID subscribe(const Event::Type& event, std::function<void(const Event&)>&& call)

as it seems the Event::Type& upsets the compiler, which I'm highly suspicious as to not to be valid C++.

Obviously, my Event struct has a Type:

typedef struct
{
    Point at;
    enum class Type
    {
        MOVE,
        LEFT_DOWN,
        LEFT_UP,
        RIGHT_DOWN,
        RIGHT_UP
    } event;
} MouseEvent;

Any suggestions? I'm currently using VS 2013.

EDIT1: Based on Alejandro's feedback, which has good points I reworked it as:

namespace observable
{
    template<typename Event>
    class Subject
    {
        using EventType = typename Event::Type;

    public:
        virtual int subscribe(const EventType& event, std::function<void(const Event&)>&& call)
        {
            observers_[event].emplace_back(std::move(call), ++last_id_);
            return last_id_;
        }

        virtual void unsubscribe(const EventType& event, int subscription)
        {
            auto& subscribers = observers_[event];
            auto to_remove = std::remove_if(subscribers.begin(),
                                            subscribers.end(),
                                            [=](const std::pair<std::function<void(const Event&)>, int>& observer) {
                return observer.second == subscription;
            });
            if (to_remove == subscribers.end())
            {
                throw std::exception("Invalid subscription");
            }
            subscribers.erase(to_remove);
        }

    protected:
        void fire(const Event& evt)
        {
            auto& subscribers = observers_[evt.event];
            for (const auto& obs : subscribers) {
                obs.first(evt);
            }
        }

    protected:
        std::unordered_map<EventType, std::vector<std::pair<std::function<void(const Event&)>, int>>> observers_;
        int last_id_ = 0;
    };
}

And a simple test that shows / proves it's intended usage:

typedef struct
{
    bool is_interesting;
    enum class Type
    {
        MOVE,
        ENTER,
        LEAVE
    } event;
} MyEvent;


class TestObservable : public observable::Subject <MyEvent>
{
public:
    void fire_move() {
        MyEvent evt = { true, MyEvent::Type::MOVE };
        fire(evt);
    }
};

void test_observable()
{
    int observer_1_move_fired = 0;
    int observer_2_move_fired = 0;

    TestObservable test_subject;

    int subscription_1 = test_subject.subscribe(MyEvent::Type::MOVE, [&](const MyEvent& evt) {
        assert(evt.event == MyEvent::Type::MOVE);
        observer_1_move_fired++;
    });

    int subscription_2 = test_subject.subscribe(MyEvent::Type::MOVE, [&](const MyEvent& evt) {
        assert(evt.event == MyEvent::Type::MOVE);
        observer_2_move_fired++;
    });

    test_subject.fire_move();
    assert(observer_1_move_fired == 1);
    assert(observer_2_move_fired == 1);

    //observer 2 should not receive MOVE events anymore...
    test_subject.unsubscribe(MyEvent::Type::MOVE, subscription_2);

    test_subject.fire_move();
    assert(observer_1_move_fired == 2);
    assert(observer_2_move_fired == 1);
}

Only consideration is that the event struct must have an enum class Type and be named event, but this can be easily relaxed / refactored if desired, certainly a comment in the template about it would be a sensible thing to do :)

And yes std::exception is a placeholder for a more appropriate one.

Thanks! great tips!

\$\endgroup\$
  • \$\begingroup\$ I'll write up an answer later with more explanation, but some things I noticed that could be improved are: typdef struct{...}name; could be replaced by struct name{...};, in your subscribe function you're taking a temporary std::function but not further moveing it, you could use emplace_back directly instead of push-ing back a temporary, and perhaps consider the erase-remove in the unsubscribe function. \$\endgroup\$ – Alejandro Jul 19 '15 at 23:11
5
\$\begingroup\$

There are a few things I'd consider changing in your code, especially since you state that you'd like to transition to modern C++.

Using struct variables in C++ doesn't require prefixing them with the struct keyword as is needed in C, so your typedef struct {...} Callable2;, which I imagine you wrote so that you could get away with Callable2 foo; as opposed to struct Callable2 foo; could be changed to simply struct Callable2 {...}; without any change in the usage of that struct.

In your subscribe member function, you take as one of your parameters, an rvalue reference to std::function<void(const Event&)> - in other words, a temporary. However, you don't subsequently move the temporary so as to take advantage of possible optimizations that moveing a std::function could have as opposed to copying it. You could remedy this by initializing this_call with {std::move(call),++last_id_}. However, even with this, you'll still end up pushing back a copy of the Callable2 you just created and effectively nullify the benefit of moveing the std::function. An alternative approach you could perhaps consider would be:

Callable2 this_call = { std::move(call), ++last_id_ };
observers_[event].push_back(std::move(this_call));
return last_id_;

Which avoids the copy of the std::function into the vector. But this still requires a move of a Callable2. Perhaps an even better approach would be:

observers_[event].emplace_back(std::move(call),++last_id_);
return last_id_;

And the only move done is on the std::function. This would require you, however, to add the appropriate constructor within Callable2, but it does the least amount of operations regarding copying and moving.

Your unsubscribe function could also benefit from the erase-remove idiom, which is by no means "modern" advice but still does the job possibly more efficiently and becomes much more readable and easier to maintain. By looking at unsubscribe it appears you want to purge an event with a specific ID and signal some emergency (which you're doing by asserting false) if the event doesn't exist. With the erase-remove idiom, unsubscribe would instead look like this:

auto& subscribers = observers_[event];
auto to_remove = std::remove_if(subscribers.begin(),subscribers.end(),[sub=subscription](const auto& observer)
                 {
                    return observer.id == sub;
                 });
if(to_remove == subscribers.end())
{
   // consider throwing an exception instead of assert(false)
}
subscribers.erase(to_remove,subscribers.end());

This more clearly expresses your intent than manually looping, comparing, etc. You can even opt for std::find_if which stops as soon as it finds the first occurrence of a condition you specify, which might be more efficient than std::remove_if depending upon the size of your events vector. I also mentioned throwing an exception rather than doing a blatant assert(false) for a whole host of reasons. I point you to Jon Kalb's excellent 3-part talk at CppCon for matters regarding exceptions, exception safe code, and its benefits.

You didn't quite express if your events need to be ordered. In the case that they don't, consider using a std::unordered_map to map your Events, which provides a hash-table container as opposed to a balanced binary search tree that std::map gives you. This would also require that EventType be hashable.

On a minor note, at the beginning of your code, you're typedef-ing a SUBSCRIPTION_ID for an int, but there's honestly little need for typedefs in modern C++ unless you're working with an older codebase. An alias declaration not only gives you the same semantics and behavior as a typedef, but also comes in handy if you ever want to have alias templates, which requires more boilerplate code if it were done with the equivalent typedef, not to mention being more readable. Use using SUBSCRIPTION_ID = int; to replace your current typedef.

To answer your questions,

  • Holding a reference (even if its a const ref) would limit how freely you can use this struct. Namely, it becomes unassignable, thus makes it unusable with std::remove_if, among other limitations. The alternative, which is to store a pointer, becomes unsafe as soon as that std::function goes out of scope and destructs.
  • Since Event::Type names a type, you need to prefix it with typename. A solution would be to create an alias declaration using EventType = typename Event::Type; within your class. For more info on typename see here
\$\endgroup\$

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.