C++ Observer design pattern implementation

Question

I'm using the Observer design pattern in order to manage events in the game I'm currently developing.

I based myself on the implementation demonstrated here but improved it in order to ease its use.

class Observer
{
public:
    virtual ~Observer() {}

    virtual void onNotify(Subject * entity, Event event) = 0;

private:
};

class Subject
{
public:
    Subject() {}

    virtual ~Subject() {}

    void addObserver(Observer* observer)
    {
        if (std::find(_observers.begin(), _observers.end(), observer) == _observers.end())
        {
            _observers.push_back(observer);
        }
    }

    void removeObserver(Observer* observer)
    {
        std::list<Observer*>::iterator it = std::find(_observers.begin(), _observers.end(), observer);
        if (it != _observers.end())
        {
            *it = NULL;
            _eraseQueue.push(it);
        }
    }

protected:
    // I'm passing the Subject, because it allows me to trigger an event
    // on a newly created object, from another class :
    // this->notify(new MyClass(...), MYCLASS_CREATED);
    // It allows me to generate new entities easily
    void notify(Subject * entity, Event event)
    {
        for (std::list<Observer*>::iterator it = _observers.begin(); it != _observers.end(); ++it)
        {
            if (*it != NULL)
                (*it)->onNotify(entity, event);
        }

        while (!_eraseQueue.empty())
        {
            _observers.erase(_eraseQueue.front());
            _eraseQueue.pop();
        }
    }

    void notify(Subject * entity, Event event, Observer* observer)
    {
        if (observer != NULL)
            observer->onNotify(entity, event);
    }

private:
    std::list<Observer*>                        _observers;
    std::queue<std::list<Observer*>::iterator>  _eraseQueue;
};

template <typename T>
class EventHandler : public Observer
{
public:
    virtual ~EventHandler() {}

    virtual void onNotify(Subject * entity, Event event)
    {
        if (dynamic_cast<T*>(this))
        {
            auto it = _actions.find(event);

            if (it != _actions.end())
            {
                (dynamic_cast<T*>(this)->*(it->second))(entity);
            }
        }
    }

protected:
    template <typename U>
    U safe_cast(Subject* entity)
    {
        if (dynamic_cast<U>(entity))
            return (dynamic_cast<U>(entity));
        else
            throw std::exception("Event thrown on not-matching entity");
    }

protected:
    std::map<const Event, void (T::*)(Subject *)>   _actions;
};

And here's some explanations about what makes it special:

• When removing an element from the observers list of a Subject, I'm storing the observers to remove into an erase queue, so that the iterator in the notify method still works even when removing an observer inside of the notify call.
  • "Observers" don't inherit from Observer directly, but inside they inherit from EventHandler which is a templated class that allows me to automatically call the corresponding pointers to function when receiving an event, without having to manage it in every Observer's daugther classes.
  • The EventHandler class implements a safe_cast method so that I can convert the Subject received from onNotify can be easily and safely casted to their inherited class.

So, maybe you wonder why I'm using a EventHandler class instead of inheriting directly from the Observer class, but here's a quick example to show you why it's useful and makes the code much easier to read:

MyClass::MyClass()
{
    _actions[GAME_STARTED] = &MyClass::gameStarted;
    _actions[CHARACTER_MOVED] = &MyClass::characterMoved;
}

void MyClass::gameStarted(Subject * entity)
{
    Game* game = safe_cast<Game*>(entity);

    // Do actions on the Game instance
}

void MyClass::characterMoved(Subject * entity)
{
    Character* character = safe_cast<Character*>(entity);

    // Do actions on the Character instance
}

That's it. My event handling is completely hidden from the final user, he just has to store the events that he wants to listen in his class, and it will automatically be handled by the EventHandler class.

I already know a few ways of improving my implementation of the Observer pattern:

• Using smart pointers,
• Actually, using references instead of pointers,
• Handle the case where the Subject instance gets deleted during the notify method call.

To clarify the role of the EventHandler class, here's how I inherit from it:

class MyClass : public EventHandler<MyClass>

Then, some explanations about the onNotify method (the safe_cast method is not really the main purpose of this class and seems to be pretty clear) :

virtual void onNotify(Subject * entity, Event event)
{
    // Verify that the 'this' pointer is of type T
    if (dynamic_cast<T*>(this))
    {
        // Find the pointer to function corresponding to the 'event' Event if it has been registered
        auto it = _actions.find(event);

        // Check if the pointer to function has been found
        if (it != _actions.end())
        {
            // Calls the method on the dynamically casted 'this' pointer, so that it can
            // generically call private methods on any class inheriting from EventHandler
            (dynamic_cast<T*>(this)->*(it->second))(entity);
        }
    }
}

And an example of how I'm using this:

// MyClass is inheriting from a 'self-templated' EventHandler
class MyClass : public EventHandler<MyClass>
{
public:
    MyClass() {
        // I'm registering in the EventHandler _actions tab the
        // events that this particular class is listening to
        _actions[CHARACTER_MOVED] = &MyClass::characterMoved;
        _actions[CHARACTER_DIED] = &MyClass::characterDied;

        // Some other initialization
    };
    virtual ~MyClass(){}

private:
    void characterMoved(Subject* entity) {
        // I'm converting the generic Subject object to a Character object
        Character* character = safe_cast<Character*>(entity);

        // I'm doing whatever I want with the character that triggered the event
        std::cout << character->getName() << " moved !" << std::endl;
    }
    void characterDied(Subject* entity) {
        // I'm converting the generic Subject object to a Character object
        Character* character = safe_cast<Character*>(entity);

        // I'm doing whatever I want with the character that triggered the event
        std::cout << "Oh no ! " << character->getName() << " died ! RIP" << std::endl;
        delete character;
    }
}

This saves me from implementing the onNotify method in every single class inheriting from Observer, and it's making the handling of the events completely hidden from the end user, that only has to register the events its class is listening to in its constructor.

But I'd like to have your opinion about the other things that I could improve, and your overall feel about this implementation.

Answer 1

Prefer not to use pointers.

Pointers do not portray ownership. So users don't know if they should pass a real pointer (to dynamically allocated objects) or if they can give the address of an object. So you have to look at the code (or documentation) to understand how to call the code.

// Subject is a reference
virtual void onNotify(Subject& entity, Event event) = 0;

// Since you registered on a subject the subject 
// will never been NULL so you can safely use a reference.

Same applies to Subject when you register the Observer

// The observer can never be NULL  so you can safely pass by reference.
void addObserver(Observer& observer)

// Note:
// You still store the observer as a pointer in your container.
std::list<Observer*>                        _observers;

// So just get the address from the reference.

Remove Observer Question

When you remove an observer why are you storing the iterate in an erase queue?

I don't think you need to do that.

    {
        *it = NULL;
        _eraseQueue.push(it);
    }

Just read your explanation. That's fair. I might have done it differently. Get the onNotify() method to return a special value that means remove from the observer queue.

Notify Question

Why are you passing subject to the notify method?

void notify(Subject * entity, Event event)

Is this not the notify on the current subject. So the interface would be:

void notify(Event event)

Then you call the onNotify() of all the observers and pass *this as the subject member to the all.

Event Handler

I can't quite work out what is happening in the event handler. I am sure its fine but I think is something will need some comments and examples when you publish the library (even for yourself because in 6 months it will be hard to remember what you were thinking when you wrote it).

Safe cast.

Not sure you need to do that.

A dynamic_cast<X>(a) will throw if a is not of type X or derived from X. But dynamic_cast<X*>(a) will never throw but return nullptr if a is not of type X or derived from X (pointer).

So you can achieve the same effect with references rather than pointers.

Prefer std::function

You are storing pointers to member functions. This is OK but a bit old style. It is "becoming" more normal to use std::function<> and store lambdas.

std::map<const Event, void (T::*)(Subject *)>   _actions;

// I would use:
std::map<const Event, std::function<void(Subject&)>>   _actions;

Underscore

Personally I don't like prefixing identifiers with '_'. The rules about its use as a prefix is non trivial. Most people don't know the rules so even if you do know the rules other people have to think about it (or look them up) to make sure you did not screw up.

Thus prefer not to use '_' as a prefix on an identifier.

What are the rules about using an underscore in a C++ identifier?

Register a method with std::function

// In EventHandler:
std::map<const Event, std::function<void(Subject&)>>   _actions;

// Register Handler (in the `Character class`)
_actions[TAKE_DAMAGE] = std::bind(&Character::takeDamage, this);

// So using the `std::function<>` technique you can use methods and
// lambdas. Because the code for calling the code is encapsulated in
// the standard function you don't need to handle the dangerous code
// involved with casting (this is done for you).

Calling the code is now.

auto find = _actions.find(event);
if (find != _actions.end()) {
    find->second(subject, event);
}

Example:

Notice below:

• A call to register an event handler for a method.
• A call to register a lambda.
• No need for dynamic_cast (or safe cast that does the same thing).
• No "Curiously recurring template pattern"
• No run time type checking (on event handlers)

Example:

#include <vector>
#include <unordered_map>
#include <iostream>
#include <typeindex>
#include <algorithm>
#include <stdexcept>
#include <functional>

class Subject;
class Event
{
};

enum class NotifyAction { Done, UnRegister};
class Observer
{
    public:
        virtual ~Observer() {}
        virtual NotifyAction onNotify(Subject& subject, Event const& event) = 0;
};
class Subject
{
    public:
        void registerObserver(Observer& observer)
        {
            if (std::find(std::begin(observers), std::end(observers), &observer) == std::end(observers)) {
                throw std::runtime_error("registerObserver(): observer already registered");
            }
            observers.push_back(&observer);
        }
        void unregisterObserver(Observer& observer)
        {
            observers.erase(std::remove(std::begin(observers), std::end(observers), &observer), std::end(observers));
        }
        void notifyObservers(Event const& event)
        {
            std::vector<Observer*>  deadObservers;
            for(Observer* observer: observers) {
                if (observer->onNotify(*this, event) == NotifyAction::UnRegister) {
                    deadObservers.push_back(observer);
                }
            }

            // Remove the dead observers.
            auto newEnd    = std::end(observers);
            for(Observer* dead:deadObservers) {
                newEnd  = std::remove(std::begin(observers), newEnd, dead);
            }
            observers.erase(newEnd, std::end(observers));
        }
    private:
        std::vector<Observer*>      observers;
};
class EventHandler: public Observer
{
    public:
        NotifyAction onNotify(Subject& subject, Event const& event) override
        {
            auto find = handlers.find(std::type_index(typeid(event)));
            if (find != handlers.end()) {
                find->second(subject, event);
            }

            return NotifyAction::Done;
        }
        template<typename T>
        void registerEventHandler(std::function<void(Subject&, Event const&)> handler)
        {
            handlers[std::type_index(typeid(T))] = handler;
        }

    private:
        std::unordered_map<std::type_index, std::function<void(Subject&, Event const&)>> handlers;
};
class MouseMove: public Event {};
class MouseClick: public Event {};

class MouseHandler: public EventHandler
{
    private:
        void mouseClick(Subject&, Event const&)
        {
            std::cout << "Mouse Clicked\n";
        }
    public:
        MouseHandler()
        {
            // Register a call to a member method
            registerEventHandler<MouseClick>(std::bind(&MouseHandler::mouseClick, this, std::placeholders::_1, std::placeholders::_2));
        }
};
class Mouse: public Subject
{
};
int main()
{
    MouseHandler    mouseEventHandler;
    // Register a call to a lambda
    mouseEventHandler.registerEventHandler<MouseMove>([](Subject&, Event const&){std::cout << "Mouse Moved\n";});

    Mouse           mouse;
    mouse.registerObserver(mouseEventHandler);

}

Answer 2

Loki makes good points, I think we can stretch it a bit more.

There's a couple of things to consider when looking at a function object, one of the neat things in c++11 and so are lambdas, in my humble attempt ended up supporting this kind of code:

container->get_root()->subscribe(WindowEvent::ACTIVATED, [&]() {
    draw_title();
    drag->update();
});

This brings up an extra consideration in the observable class, as a lambda is an anonymous function wrapped into a function object the unsubscribe method wont work as it is written. You could assign the lambda to a variable and then pass that to the subscribe method or handle the unsubscribe in a different way. Personally i'm 'experimenting' a version that returns a 'subscription id' to be used as a parameter to unsubscribe.

Another thing to consider is a 'templatized' version of the observable pattern, most stuff will be compiled right on avoiding some overhead and reducing quite some code once you start using it for different event types and so.

You can find my humble attempt of the 'templatized' version in another review request: Observable template in modern C++