views:

197

answers:

4

Hey,

I'm working on event handling in C++ and to handle notification of events, I have a class EventGenerator which any class generating events can inherit from. EventGenerator has a method which other classes can use to add in callbacks and a method to call the callbacks once an event happens

To handle notification of different types of events, I've parametrized EventGenerator on template type T and the notifier class can then inherit from EventGenerator multiple times parametrized on different types.

For the sake of completeness, here's the code for EventGenerator

#ifndef _EventGenerator
#define _EventGenerator

#include <list>

#include "EventListener.h"

template <class Event>
class EventGenerator {
private: 
    std::list<EventListener<Event>*> listeners;
protected:
    EventGenerator() {}

    void changeEvent(Event event) {
        std::list<EventListener<Event>*>::const_iterator it = listeners->begin();
     for (; it != listeners->end(); it++) {
      (*it)->changeEvent(event);
     }
    }

public:
    void addListener(EventListener<Event>* listener) {
         listeners->push_back(listener);
        }
};

#endif



and here's the code for EventListener which any class which wants to add callbacks inherits from -

#ifndef _EventListener
#define _EventListener

template <class Event>
class EventListener {
private:
    EventListener(const EventListener<Event>& event);
protected:
    EventListener() {}
public:
    virtual void changeEvent(Event event) = 0;
};

#endif



I've a feeling this is not a very good design and was wondering if there was a better design out there for such a problem.

Edit: What bothers is the fact that I'm using multiple inheritance. I've been frequently warned against using it so I guess I wanted opinions on whether such a design could lead to bad things happening in the future

Thanks Sid

A: 

It's bad design because in your case things probably aren't going to work when you dispatch events... Unless you have some clever meta-programming way to iterate over all the types you're derived from.

In general though, I don't think there's anything wrong with multiple inheritance in general, or of this sort.

Autopulated
The iteration when dispatching events is done by the changeEvent method of the EventGenerator class. When a particular type of event is generated, the changeEvent method of that event type (inherited from the EventGenerator of that event type) will be called.
Sid
Then I withdraw the only objection I could think of!
Autopulated
+1  A: 

Beware of diamond inheritance heirarchies. Also note that overloading virtual functions is a bad thing. So if you have something like this:

class Handler : public EventHandler<int>, public EventHandler<string> { ... };

Which changeEvent() function will be called? Don't count on it!

If you are careful the above code should be fine, but if you want to avoid inheritance altogether then I suggest using function references associated with some unique identifier. As an example:

class Listener { public: virtual ~Listener ( ) { } };
template<typename Event> class Distributor : public Listener {
    public:
    void addListener (shared_ptr<Listener>, function<void (Event)>);
    void listen (Event e) { 
        for_each(_listeners.begin(), _listeners.end(), 
            bind(&ListenNode::listen, _1, e));
    }
    private:
    struct ListenNode { 
        weak_ptr<Listener> listener;
        function<void (Event)> callback;
        void listen (Event e) {
            shared_ptr<Listener> l = listener.lock();
            if(l) callback(e);
        }
    };
    list<ListenNode> _listeners;
};

With this setup, all listeners derive from one base class virtually. Listeners can have multiple callbacks registered, and Distributors can be chained. Of course you don't have to use shared_ptr's but I like them because they save from the hassle of unregistering listeners. You can register the callbacks any way you like, associating them with a string, integer or whatever.

I have omitted a lot of detail, event distribution is a complicated business. I think Andrei Alexandrescu wrote a detailed article on the topic, look it up.

A: 

You might run into problems as you make classes that are derived on multiple classes that use events, should those parent classes use the same events.

Of other note, you will need to use the following syntax to define your abstract callback function:

template<class T>
class Base
{
    virtual void listener() = 0;
}

class Derived
    : public class Base<int>
    , public class Base<float>
{
    void Base<int>::listener(){...}
    void Base<float>::listener(){...}
}

If you then need to call either of those from Derived, you probably would need to use similar syntax (Although I honestly don't know), but if you call it from a Base or Base reference, it'll all work nicely. As an added bonus, you won't compile unless the listener functions are actually defined.

More or less, multiple inheritance means you need to pay more attention, but it's a valuable feature of the language. I think there might also be more template shenanigans you can do to prevent diamond inheritance, at the cost of elegance.

Narfanator
+1  A: 

As others have said you might run into diamond inheritance issues. Also, inheriting from an event handler base can violate Single Responsibility Principle. What I would do is use a nested class inside a class that needs to know about some event:

class CNeedsToHandleEvent {
//...
private:

  void OnChange (Event event) {
    //Do processing of the event
  }

  class ChangeEventHandler : public EventListener {
    virtual void changeEvent(Event event) {
      CNeedsToHandleEvent* parent = OUTERCLASS(CNeedsToHandleEvent, m_ChangeEventHandler);
      parent->OnChange(event);
    }
  } m_ChangeEventHandler;

  friend class ChangeEventHandler;
};

And here is the OUTERCLASS macro. Some might consider its use controversional and perhaps it may have portability issues, but it's pretty convinient:

// Get a pointer to outer class of a nested class
#ifndef OUTERCLASS
#define OUTERCLASS(className, memberName) \
    reinterpret_cast<className*>(reinterpret_cast<unsigned char*>(this) - offsetof(className, memberName))
#endif

You can always instantiate the nested class with a pointer to the parent class so that it can call the real handlers instead of relying on the macro.

Igor Zevaka