C++ Observer design pattern implementation

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::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::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 _observers;
std::queue::iterator> _eraseQueue;
};

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

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

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                        _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(a) will throw if a is not of type X or derived from X. But dynamic_cast(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   _actions;

// I would use:
std::map>   _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>   _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
#include
#include
#include
#include
#include
#include

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

StackExchange Code Review Q#92199, answer score: 16