views:

83

answers:

5

I am busy adding a generic observer mechanism to a legacy C++ application (using Visual Studio 2010, but not using .Net, so .Net delegates are out of the question).

In the design I want to separate the application-specific part as much as possible from the generic observer mechanism.

The most logical way of implementing observers seems this way:

class IDoThisObserver
   {
   public:
      void handlDoThis(int arg1, int arg2) = 0;
   };

For every type of observer (IDoThisObserver, IDoThatObserver, ...) the arguments of the methods (handleDoThis, handleDoThat) are different.

What remains in a generic way of storing the observers, like this:

template<typename T>
class ObserverContainer
   {
   public:
      void addObserver (T &t) {m_observers.push_back(&t);}
   private:
      std::list<T*> m_observers;
   };

Calling an observer can't be generalized since the arguments are different for every observer type.

An alternative way would be to 'pack' all arguments into one argument, like this:

struct DoThisInfo
   {
   DoThisInfo (int arg1, int arg2) : m_arg1(arg1), m_arg2(arg2) {}
   int m_arg1;
   int m_arg2;
   };

And then define a more generic observer, like this:

template<typename T>
class IObserver
   {
   public:
      void notify(const T &t) = 0;
   };

And a collection of these observers would then become this:

template<typename T>
class ObserverContainer
   {
   public:
      void addObserver (IObserver<T> &obs) {m_observers.push_back(&obs);}
   private:
      std::list<IObserver<T>*> m_observers;
   };

Now, much more logic can be centrally added to this ObserverContainer, including calling all observers. The 'initiator' of the call only needs to create and fill in the notification structure.

Classes that want to inherit from multiple kinds of observers, need to do it like this:

class MyObserver : public IObserver<NotifyThis>, public IObserver<NotifyThat>
   {
   ...
   };

Which of these approaches (observers with multiple explicit arguments or with one struct argument) seems the best? Are there any advantages or disadvantages to either of these approaches?

EDIT: I looked a bit further to alternative approaches, and the Slot/Signal approach seems another good candidate. Are there any important disadvantages in Slot/Signal that I should know of?

+2  A: 

Why not just do:

class IObserver {
    // whatever is in common
};

class IDoThisObserver : public IObserver
{
   public:
      void handlDoThis(int arg1, int arg2) = 0;
};

class IDoThatObserver : public IObserver
{
   public:
      void handlDoThat(double arg1) = 0;
};

?

Then you have:

class ObserverContainer
{
   public:
      void addObserver (IObserver* t) {m_observers.push_back(t);}
   private:
      std::list<IObserver*> m_observers;
};
Kornel Kisielewicz
That's just putting common code in IObserver. I was looking for a way of putting generic code in the ObserverContainer.
Patrick
This approach is good as long as there is no need to iterate through the observers and call their handling methods in some general-ish way. That would be a nightmare.
manneorama
@Patrick, what kind of code would you like to have there?
Kornel Kisielewicz
The problem with this is that the `ObserverContainer` can't call the `handle` methods on the observers without a big mess of casts.
bshields
@bshields, how can he cast handle if he doesn't know what parameters do they take?
Kornel Kisielewicz
@Kornel The primary function of the `ObserverContainer` would seem to be to iterate through the list of observers and call the `notify` / `handle` method on each one. The template version of `IObserver` from the original question would support this nicely. You're version doesn't, the `ObserverContainer` would have to cast each `IObserver*` to an `IDoThisObserver` or an `IDoThatObserver` in order to be able to call the appropriate `handle` method. This is obviously not the right approach.
bshields
@bshields, how would the ObserverContainer know what to cast to? Unless you have reflection there's no way to get the proper method, and the parameters it should get anyway. Unless the ObserverContainer would have hardcoded cases for every type of Observer, but that boils down to some form of RTTI anyway.
Kornel Kisielewicz
@Kornel Yeah my point exactly, it would require hard-coded cases for every type of Observer + `dynamic_cast`. I'm not advocating it, just pointing out that that's what it would take to give the `ObserverContainer` the presumably intended functionality in the code you wrote.
bshields
+1  A: 

The design with the struct argument is definitely better as it allows for generic code to be written in the ObserverContainer. It's generally a good design practice to replace longish argument lists with objects that encapsulate the arguments and this is a good example of the payoff. By creating a more general abstraction for your notify method (with the struct you're defining notify as a method that takes a chunk of "data" whereas with the arg list you're defining a method that takes two numbers) you allow yourself to write generic code that uses the method and doesn't have to concern itself with the exact composition of the passed in chunk of data.

bshields
A: 

I wouldn't get the syntax right so I'm just going to list the declarations to illustrate the structures. A generic Observer could be made to expect a parameter that is either subclassed to specific forms of your required parameters or is struct including a horizontal mapping of all primitive parameters that will be required by your Observers. Then the ObserverContainer could function as an AbstractFactory and each subclass of the ObserverContainer could be DoThatObserverFactory and DoThisObserverFactory. The factory would build an observer and assign a configuration to the observer to tell it which parameter to expect.

class AbstractObserverFactory {...};
class DoThatObserverFactory : AbstractObserverFactory {...};
class DoThisObserverFactory : AbstractObserverFactory {...};
class ObserverParam {...};
class DoThatObserverParam : ObserverParam {...};
class DoThisObserverParam : ObserverParam {...};
class Observer;
class DoThisObserver : public Observer
{
   public:
      void handlDoThis(DoThisObserverParam);
};
CitizenForAnAwesomeTomorrow
+1  A: 

I don't think either of your approaches would fit your requirement as is. However a little modification using a DataCarrier containing the dataset passed across all the observers wherein each observer would know what to read would do the trick. The sample code below might clear it (note i have not compiled)

 enum Type {
    NOTIFY_THIS,
    NOTIFY_THAT
 };

 struct Data {
 virtual Type getType() = 0;
 };

 struct NotifyThisData: public Data {
    NotifyThisData(int _a, int _b):a(_a), b(_b) { }
    int a,b;
    Type getType() { return NOTIFY_THIS; }
 };

 struct NotifyThatData: public Data {
    NotifyThatData(std::string _str):str(_str) { }
    std::string str;
    Type getType() { return NOTIFY_THAT; }
 };

 struct DataCarrier {
    std::vector<Data*> m_TypeData;  
 };

 class IObserver {
 public:
     virtual void handle(DataCarrier& data) = 0;
 };

 class NotifyThis: public virtual IObserver {
 public:
         virtual void handle(DataCarrier& data) {
                 vector<Data*>::iterator iter = find_if(data.m_TypeData.begin(), data.m_TypeData.end(), bind2nd(functor(), NOTIFY_THIS);
                 if (iter == data.m_TypeData.end())
                         return;
                 NotifyThisData* d = dynamic_cast<NotifyThisData*>(*iter);
                 std::cout << "NotifyThis a: " << d->a << " b: " << d->b << "\n";
         }
 };

 class NotifyThat: public virtual IObserver {
 public:
         virtual void handle(DataCarrier& data) {
                 vector<Data*>::iterator iter = find_if(data.m_TypeData.begin(), data.m_TypeData.end(), bind2nd(functor(),NOTIFY_THAT);
                 if (iter == data.m_TypeData.end())
                         return;            
                 NotifyThatData* d = dynamic_cast<NotifyThatData*>(*iter);
                 std::cout << "NotifyThat str: " << d->str << "\n";
         }
 };

 class ObserverContainer
    {
    public:
       void addObserver (IObserver* obs) {m_observers.push_back(obs);}
       void notify(DataCarrier& d) {
                 for (unsigned i=0; i < m_observers.size(); ++i) {
                         m_observers[i]->handle(d);
                 }
         }
    private:
       std::vector<IObserver*> m_observers;
    };

 class MyObserver: public NotifyThis, public NotifyThat {
 public:
         virtual void handle(DataCarrier& data) { std::cout << "In MyObserver Handle data\n"; }
 };

 int main() {
         ObserverContainer container;
         container.addObserver(new NotifyThis());
         container.addObserver(new NotifyThat());
         container.addObserver(new MyObserver());

         DataCarrier d;
         d.m_TypeData.push_back(new NotifyThisData(10, 20));
         d.m_TypeData.push_back(new NotifyThatData("test"));

    container.notify(d);
    return 0;
 }

This way u need to modify only the enum if u add a new structure. Also u can use boost::shared_ptr to handle the mess of pointers.

aeh
+1  A: 

Have you looked into Boost.Signals? Better than to reimplement the wheel.

As for Parameters: Calling an observer/slot should conceptionally be the same as if you would call an ordinary function. Most SignalSlots-Implementations allow multiple Parameters, so use it. And please use different signals for different observer types, then there is no need to pass around data in Variants.

Two Disadvantages of the Observer-Pattern/SignalSlots i have seen:
1) Program flow is difficult or even impossible to understand by looking only at the source.
2) Heavily dynamic programs with lots of Observers/SignalSlots may encounter a "delete this"

Everything aside, i like Observers/SignalSlots more than subclassing and thus high coupling.

Markus Kull