Observable template in modern C++

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

typedef int SUBSCRIPTION_ID;

template
    class Subject2
    {
        typedef struct
        {
            std::function method;
            int id;
        } Callable2;

    public:
        virtual SUBSCRIPTION_ID subscribe(const EventType& event, std::function&& 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& 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> 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 th

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 - 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

StackExchange Code Review Q#97360, answer score: 5