C++ implementation of the observer design pattern

Question

Started learning design patterns using C++. Below is an implementation of the Observer Pattern. Two abstract class templates Observer and Subject define the required interfaces. Two specific implementation ConcreteObserver and ConcreteSubject are defined to test the interfaces (I have not found good generic real-life examples that I like for the Observer Pattern, and so decided to go with ConcreteWhatever).

In this implementation, the subject pushes the state change to the observers.

#include <iostream>
#include <set>

// interfaces

template <typename T>
class Observer {
public:
    virtual void update(T subjectState)=0;
};

template <typename T>
class Subject {
public:
    virtual void registerObserver(Observer<T>* ptr_observer) = 0;
    virtual void unregisterObserver(Observer<T>* ptr_observer) = 0;
    virtual void notifyObservers() = 0;
};

// implementations

template <typename T>
class ConcreteObserver : public Observer<T> {
    Subject<T>* ptr_subject;
    T subjectState;
public:
    ConcreteObserver(Subject<T>* ptr_subject) : ptr_subject(ptr_subject) {
        std::cout << "Concrete observer " << this << " created to observe " << ptr_subject << std::endl;
        ptr_subject->registerObserver(this);
    }

    void update(T subjectState) {
        this->subjectState = subjectState;
        std::cout << "Observer " << this << " notified of subject " << ptr_subject << " state change to " << subjectState << std::endl;
    }

    void currentSubject(Subject<T>* ptr_subject) {
        ptr_subject->registerObserver(this);
        this->ptr_subject = ptr_subject;
    }

    ~ConcreteObserver() {
        ptr_subject->unregisterObserver(this);
    }
};

template <typename T>
class ConcreteSubject : public Subject<T> {
    std::set<Observer<T>*> observers;
    T state;
public:
    ConcreteSubject(T state) : state(state) {
        std::cout << "Concrete subject " << this << " created with state " << state << std::endl;
    }

    void registerObserver(Observer<T>* ptr_observer) {
        observers.insert(ptr_observer);
        std::cout << "Subject " << this << " registered observer " << ptr_observer << std::endl;
    }

    void unregisterObserver(Observer<T>* ptr_observer) {
        observers.erase(ptr_observer);
        std::cout << "Subject " << this << " unregistered observer " << ptr_observer << std::endl;
    }

    void notifyObservers() {
        for (std::set<Observer<T>*>::iterator iter = observers.begin(); iter != observers.end(); iter++) {
            (*iter)->update(state);
        }
    }

    void stateChanged() {
        notifyObservers();
    }

    T getState() {
        return state;
    }

    void setState(T state) {
        this->state = state;
        std::cout << "State of subject " << this << " changed to " << state << std::endl;
        stateChanged();
    }
};

int main() {
    std::cout << "Testing Subject-Observer implementation:" << std::endl;

    ConcreteSubject<int>* ptr_subject = new ConcreteSubject<int>(42);

    ConcreteObserver<int>* ptr_observer_1 = new ConcreteObserver<int>(ptr_subject);
    ConcreteObserver<int>* ptr_observer_2 = new ConcreteObserver<int>(ptr_subject);
    ConcreteObserver<int>* ptr_observer_3 = new ConcreteObserver<int>(ptr_subject);

    ptr_subject->setState(19);

    ptr_subject->unregisterObserver(ptr_observer_3);

    ptr_subject->setState(13);

    delete ptr_subject;

    getchar();
}

Answer 1

Design Review

Ownership

One of the advances of C++ over is C is ownership semantics. We understand who owns an object (and thus who is responsible for destroying the object). We do this by using a set of helper classes that explicitly state ownership or pass by reference to indicate non ownership.

The trouble with pointers is that they have no ownership semantics. You can't tell from their usage who is supposed to own the pointer and thus you can not tell who is responsible for deleting the object. Thus in C++ it is highly discouraged to use RAW pointers in public interfaces (I am not saying are not used, I am saying is that their normally place is deep inside a class so their ownership is very obvious and not normally exposed in public interfaces).

In you interface you never transfer ownership and you have both a register and unregister method on the subject. So that interface can be easily changed to show the ownership is not part of the interface.

template <typename T>
class Subject {
public:
    virtual void registerObserver(Observer<T>& ptr_observer) = 0;
    virtual void unregisterObserver(Observer<T>& ptr_observer) = 0;
    virtual void notifyObservers() = 0;
};

Please note I changed the * to &. This shows you are not passing ownership but you will always pass an observer. The additional upside is that you can no longer pass nullptr as an observer (you forgot to check to see if the pointer was nullptr and thus had a potential for calling a method on a nullptr (not a good idea).

Passing by value:

Here you pass the subjectState by value.

class Observer {
public:
    virtual void update(T subjectState)=0;
};

If T is large or complex then this is probably not a good idea. Pass a reference to the object. If the subject wants a copy they can explicitly make a decision to copy the object when they receive it (they have enough context to make an intelligent choice). So pass by reference instead.

class Observer {
public:
    virtual void update(T const& subjectState)=0;
              //          ^^^^^^  Normally I would pass by const ref.
              //                  But there is a case for passing by ref
              //                  as this allows each subject to mutate the
              //                  state of the object that is detectable
              //                  by other observers.
};

Virtual Destructors

If a class has virtual methods then you should have a virtual destructor.

The compiler will warn you about this if you turn up the warning level.

Override

When you override virtual methods you should add override to the method declaration.

The compiler will warn you about this if you turn up the warning level.

Code Review

Prefer to use the range based for when looping over a container.

        for (std::set<Observer<T>*>::iterator iter = observers.begin(); iter != observers.end(); iter++) {
            (*iter)->update(state);
        }

One thing you should change is the increment. It is slightly more efficient to use prefix increment (though the compiler probably compensates automatically). But it also makes your code look more modern.

        for (std::set<Observer<T>*>::iterator iter = observers.begin(); iter != observers.end(); ++iter) {
            (*iter)->update(state);
        }

This works. But it is a bit verbose.
In C++11 we made this easier with auto which makes the compiler do the work of working out the type.

        for (auto iter = observers.begin(); iter != observers.end(); ++iter) {
            (*iter)->update(state);
        }

In C++14 we added std::begin() and std::end() functions to make the above more generic and work with arrays as well as standard containers.

        for (auto iter = std::begin(observers); iter != std::end(observers); ++iter) {
            (*iter)->update(state);
        }

Also in C++14 we added the range based for:
The below loop is basically syntactic sugar (though it only calls std::end() once in C++17) for the previous loop I wrote.

        for (auto const& item: observers) {
            item->update(state);
        }

Const correctness.

Const correctness is really hard to add in afterwords (believe me). So best to start with your classes with it already in place. So any method that does not mutate the state of the object should be marked const

    void stateChanged() const {  // I added the cost here.
        notifyObservers();
    }

This class should be const and return by reference.

    T const& getState() const { // Notice the 2 `const` on this.
        return state;
    }

Move semantics and efficiency.

Copy semantics are good for simple types. But move semantics allow you a very effecient way of adding an object to another object. So you should learn how to move or construct in place an object.

    // Your version forces two copies.
    // You have a copy when you call setState to put the object
    // into the parameter `state` then you have a second copy
    // when you assign to `this->state`
    void setState(T state) {      // Copy here
        this->state = state;      // Copy here
        stateChanged();
    }

If T was large thats a lot of extra copying.
You should replace this with three methods:

    void setState(T const& newState) {   // Pass by reference avoid copy.
        state = state;
        stateChanged();
    }

    void setState(T&& newState) {        // Pass by RValue-Ref avoid copy.
        state = std::move(newState);     // Then move over current state.
        stateChanged();
    }

    // Advanced technique
    template<typename... A>
    void setState(A&&... args) {             // Pass arguments to construct a T by ref
        state = T(std::forward<A>(args)...); // Build the state in place.
        stateChanged();
    }

Don't use this-> it hides errors.

        this->state = state;

The only reason to use this-> is to distinguish a member that has been shadowed by a local variable (like a parameter). The problem is that it is error prone.

You must always remember to use this-> to use the local member otherwise you will accidentally use the local variable. If you forget there is no compiler warning that will help you.

On the other hand the compiler will warn you when you have shadowed variables. So if you never have a shadowed variable you can never accidentally use the wrong one.

Thus it is discouraged to use this-> as you are just setting yourself up for a hidden error. Rather make the compiler warn you about shadowed variables and use an appropriate name so it does not shadow.

Prefer '\n' over std::endl

    std::cout << "Testing Subject-Observer implementation:" << std::endl;

The difference between the two is a flush of the stream (done by std::endl). Flushing done by the user is one of the main speed slow downs in C++ code over C. The streams will already flush themselves at the optimal time, so a manual flush will usually retard performance.

Prefer to use automatic variables.

    ConcreteSubject<int>* ptr_subject = new ConcreteSubject<int>(42);

This is not Java. You don't need to new objects to create them you can just declare them locally.

    // This creates a local object
    ConcreteSubject<int> ptr_subject(42);

Using pointers like this (RAW pointers) is also discouraged as it very dangerous. If an exception occurs you will leak the pointer. You also have to keep track of the object and remember to call delete on it, which is error prone (as shown by a decades worth of C code). So we usually use wrappers to make sure the delete is called automatically.

    std::unique_ptr<ConcreteSubject<int>> aPointer std::make_unique<ConcreteSubject<int>>(42);

This is a pointer held inside an object wrapper. When it goes out of scope it will automatically be destroyed, this calls the destructor, which calls delete on the contained pointer.

As an example:

    ConcreteObserver<int>* ptr_observer_1 = new ConcreteObserver<int>(ptr_subject);

I see no call to delete for this object. So you just leaked it.

Answer 2

Potential Bugs

You don't define virtual destructors for Observer and Subject. This makes code such as

Subject<int>* a = new ConcreteSubject<int>(3);
//...
delete a;

invoke undefined behavior.

The Single Responsibility Principle

This principle, which is one of the foundations of good object oriented programming, states that each class should only have a single responsibility to fulfill. In your case, however, both ConcreteObserver and ConcreteSubject seem to violate this: In addition to doing their duties as observer and subject, they also log all their actions to std::cout, which should be the job of another part of your program.

Performance Issues

Currently, you pass everything by value. However, what if your template parameter T is not cheap to copy (imagine a std::vector with millions of elements)? In that case, you're wasting time and resources by making a copy too much. Instead, you should always pass by const& (if you need more advice on when to pass by reference and when to pass by value, I suggest you take a look at the Cpp Core Guidelines).

Don't use std::endl

You usually don't need to flush the underlying output buffer, which std::endl does besides inserting a new line, and if you do, there is flush(). '\n' will do in almost all cases.