HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Still an observer pattern?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stillobserverpattern

Problem

Usually when I see tutorials about Observer Pattern I see an unique method called notify, but I'm wondering. What if I have different methods that can be called in different moments but needs to notify the others when this happen? Like events, am I doing this wrong? or still begin the observer pattern?

#include 
#include 
#include 

class Observer
{
public:
    virtual void notifyBefore() = 0;
    virtual void notifyAfter() = 0;
};

class Subject
{
public:
    void attachObserver(Observer * observer)
    {
        observers.push_back(observer);
    }

    void detachObserver(Observer * observer)
    {
        auto index = std::find(observers.begin(), observers.end(), observer);
        if (index != observers.end())
        {
            observers.erase(index);
        }
    }

    virtual void notifyBefore()
    {
        for (auto current : observers)
        {
            current->notifyBefore();
        }
    }

    virtual void notifyAfter()
    {
        for (auto current : observers)
        {
            current->notifyAfter();
        }
    }
private:
    std::vector observers;
};

class ConcreteObserver : public Observer
{
public:
    void notifyBefore()
    {
        std::cout attachObserver(new ConcreteObserver);

    subject->notifyBefore();

    for (int i = 0; i notifyAfter();
}

Solution

Yes its still the observer pattern (details in my comment on the question).

But there are a few implementation details that are bad for a C++ context.

Good interface for your observer:

class Observer
{
public:
    virtual void notifyBefore() = 0;
    virtual void notifyAfter() = 0;
};


But you should also make the destructor virtual. It is quite probable that these objects will be destroyed via a pointer to the base class. Without a virtual destructor this is undefined behavior.

You have the basics of subject correct.

class Subject
{
public:
    void attachObserver(Observer * observer)
    {
        observers.push_back(observer);
    }


My one problem is here. You are passing a pointer to an observer. Pointers do not express ownership. So it is unclear for future users of your object if you should pass a dynamically allocated object or a a pointer to an automatic object. It becomes clear by reading your class that you are not taking ownership of the pointer and your class always assumes that it is not NULL. So in this case it is probably better to pass it by reference.

If there are cases were ownership can be passed then you should use smart pointers. A quick search on google for smart pointers or ownership semantics should give you lots of references.

void attachObserver(Observer& observer)
    {
        observers.push_back(&observer);  // Save the address of the observer
                                         // It is the responsibility of the observer
                                         // to make sure it lives long enough or
                                         // detaches itself on destruction.
    }


Implementation:

class ConcreteObserver : public Observer
{
public:
    void notifyBefore()
    {
        std::cout << "You called me before..." << std::endl;
    }

    void notifyAfter()
    {
        std::cout << "You called me after..." << std::endl;
    }
};


In C++11 we added the override keyword. This should be used in case you refactor your class. This will help in finding locations were you have misnamed the functions:

void notifyAfter() override ....


This is not Java

auto subject = new ConcreteSubject;
subject->attachObserver(new ConcreteObserver);


You should prefer to use automatic objects (that is perfect here). If you are going to use new then you should wrap the result in a smart pointer to make sure the allocation is exception safe.

Code Snippets

class Observer
{
public:
    virtual void notifyBefore() = 0;
    virtual void notifyAfter() = 0;
};
class Subject
{
public:
    void attachObserver(Observer * observer)
    {
        observers.push_back(observer);
    }
void attachObserver(Observer& observer)
    {
        observers.push_back(&observer);  // Save the address of the observer
                                         // It is the responsibility of the observer
                                         // to make sure it lives long enough or
                                         // detaches itself on destruction.
    }
class ConcreteObserver : public Observer
{
public:
    void notifyBefore()
    {
        std::cout << "You called me before..." << std::endl;
    }

    void notifyAfter()
    {
        std::cout << "You called me after..." << std::endl;
    }
};
void notifyAfter() override ....

Context

StackExchange Code Review Q#64172, answer score: 4

Revisions (0)

No revisions yet.