3
\$\begingroup\$

I'm reading on design patterns for a software engineering class. I am doing small implementations of some I find the most interesting / applicable to better understand them.

I'd like a code review on my design, more particularly on the degree of coupling between my Observer and Publisher classes. I feel that the add_observer() function may be coupled too strongly with the Observer object and that there may be a way to do this with looser coupling.

Observer.h

#ifndef OBSERVER_H_INCLUDED
#define OBSERVER_H_INCLUDED

#include <string>

class Publisher;

class Observer {
public:
    virtual void eventOnProperty(Publisher* source, 
         std::string event_name, std::string value) = 0;
};

#endif

Publisher.h

#ifndef PUBLISHER_H_INCLUDED
#define PUBLISHER_H_INCLUDED

#include "Observer.h"
#include <list>
#include <string>

class Publisher {
public:
    void notify_observers(std::string, std::string);
    void add_observer(Observer*);
protected:
    std::list<Observer*> my_observers;
};

#endif

Publisher.cpp

#include "Publisher.h"
#include "Observer.h"
#include <string>
#include <list>

void Publisher::notify_observers(std::string event_name, std::string value) {
    std::list<Observer*>::iterator it;
    for(it = my_observers.begin(); it != my_observers.end(); it++) {
        (*it)->eventOnProperty(this, event_name, value);
    }
}

void Publisher::add_observer(Observer* obs) {
    my_observers.push_back(obs);
}
\$\endgroup\$
  • \$\begingroup\$ Don't have time to go into a lot of detail at the moment, but both your classes should have virtual destructors. Also, ownership semantics are somewhat unclear here. \$\endgroup\$ – Yuushi May 26 '13 at 4:28
3
\$\begingroup\$

Don't pass by pointer. There is no ownership associated with it (and it can be NULL and thus you need to test for that).

class Publisher {

    // Is the publisher taking ownership of the observer?
    void add_observer(Observer*);

Is your class taking ownership?

If the answer is yes (you take ownership and thus control lifespan). Then you should pass the object using a smart pointer that allows transfer of ownership.

    void add_observer(std::unique_ptr<Observer> obs);

If the answer is sort of. We are taking shared ownership (ie there might be other objects that are also using it). Then you pass using a smart pointer indicating shared ownership.

    void add_observer(std::shared_ptr<Observer> obs);

If the answer is NO then you pass a reference (as a reference can not be NULL) and also indicates that you don't have onership.

    void add_observer(Observer& obs);

If you do take ownership you need to do some work in the destructor of your publisher to make sure that you correctly relinquish ownership when the publisher dies.

If you don't have an ownership claim. This also means that the lifespan of the object may not live as longer as the publisher so you also need a way for the object to remove itself if it dies first and for the publisher to notify the observer that it has dies so it does not de-register itself from a dead object.

   void del_observer(Observer& obs);
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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