current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I have a observer pattern implemented and I'm using two base classes called Subject and Observer. - The Subject class is an abstract class that has methods to insert the observers. And the Observer is just an interface.

What I want is to provide a safer interface to classes that inherit to access the vector of observers and that is the way I implemented...

What would you do to make this better? Is this a bad design? Anything wrong?

#include <iostream>
#include <vector>
#include <memory>
#include <algorithm>

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

    virtual void notifyHuman() = 0;
    virtual void notifyRobot() = 0;
};

class Subject
{
    typedef std::vector<std::unique_ptr<Observer>> vector_type;
public:
    virtual ~Subject() {}

    void attachObserver(vector_type::value_type observer)
    {
        container.push_back(std::move(observer));
    }

    vector_type::const_iterator begin() const
    {
        return container.begin();
    }

    vector_type::const_iterator end() const
    {
        return container.end();
    }

    virtual void notifyHuman() = 0;
    virtual void notifyRobot() = 0;
private:
    vector_type container;
};

class FooSubject : public Subject
{
public:
    FooSubject() {}
    ~FooSubject() {}

    void notifyHuman()
    {
        std::for_each(begin(), end(), [] (auto & observer) {
            observer->notifyHuman();
        });
    }

    void notifyRobot()
    {
        std::for_each(begin(), end(), [] (auto & observer) {
            observer->notifyRobot();
        });
    }
};

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

    void notifyHuman()
    {
        std::cout << "received notification of human." << std::endl;
    }

    void notifyRobot()
    {
        std::cout << "received notification of robot." << std::endl;
    }
};

int main()
{
    FooSubject subject;
    subject.attachObserver(std::make_unique<BarObserver>());
    subject.notifyHuman();
    subject.notifyRobot();

    return 0;
}
share|improve this question

2 Answers 2

up vote 2 down vote

Looks good.

This is a little confusing:

void attachObserver(vector_type::value_type observer)

To see what I have to pass to this function. I need to look up vector_type (which is in the class). I would make it more explicit so that I can see exactly what is expected:

void attachObserver(std::unique_ptr<Oberver> observer)

Don't see the need for:

class FooSubject : public Subject

I would just put all methods in Subject (no need for a FoodSubject). But this is a pattern not a design and you may have a reason that are not obvious from the current question.

There is no need to define methods that are covered by the default versions.

// I would not bother with this
FooSubject() {}
~FooSubject() {}
share|improve this answer
    
Thanks Loki - I made FooSubject because I may want to create others subjects after... then Subject is just a type. Open/Closed principle - "should be open for extension, but closed for modification" - right? - And the vector_type I made most because of the return of begin and end - What to do if I remove? Use auto instead? (C++14)? –  yayuj Nov 15 '14 at 20:32
up vote 1 down vote

I'm going to suggest changing the interface of Observer just a bit.

class Subject;

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

    virtual void notifyHuman(Subject& subject) = 0;
    virtual void notifyRobot(Subject& subject) = 0;
};

This provides some context to the concrete sub-types of Observer. Depending on the type of the Subject and the state of the object, an observer may do different things. For example, BarObserver might have been designed to deal only with BarSubjects.

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

    void notifyHuman(Subject& subject)
    {
        BarSubject* barSubject = dynamic_cast<BarSubject*>(&subject);
        if ( barSubject )
        {
            std::cout << "received notification of human on a BarSubject." << std::endl;
        }
    }

    void notifyRobot(Subject& subject)
    {
        BarSubject* barSubject = dynamic_cast<BarSubject*>(&subject);
        if ( barSubject )
        {
            std::cout << "received notification of robot on a BarSubject." << std::endl;
        }
    }
};

The implementations of notifyHuman and notifyRobot in the Subject hierarchy have to be updated to pass *this to the observers.

void notifyHuman()
{
    std::for_each(begin(), end(), [] (auto & observer) {
        observer->notifyHuman(*this);
    });
}

void notifyRobot()
{
    std::for_each(begin(), end(), [] (auto & observer) {
        observer->notifyRobot(*this);
    });
}
share|improve this answer
    
I really liked your modifications, thank you very much. –  yayuj Nov 16 '14 at 15:31
    
Though I also like the modification. It only becomes useful if an Observer can be attached to multiple Subjects simultaneously. With Subject taking ownership of the observer that is not the case and suggests we should modify the interface even further by using a non owning Subject. –  Loki Astari Nov 17 '14 at 15:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.