2
\$\begingroup\$

I am writing a simple event handling class in C++, to avoid having delegate registration and calls cluttering the rest of the code. I have a design that seems to do the job, but it has a couple of drawbacks.

So far it looks like this:

template<typename T>
struct IDelegate { virtual void operator ()(const T & args) const = 0; };

template<typename T>
class EventHandler
{
public:
    void Trigger(const T & args) const;
    void AddListener(const IDelegate<T> & delegate);
    void RemoveListener(const IDelegate<T> & delegate);
};

Which in practice would look like this (some code elided for clarity):

class Foo
{
public:
    struct CustomEventArgs { int i; };
    EventHandler<CustomEventArgs> * SomeHandler() const { return m_handler; }

    void SomeMethod()
    {
        // Some code
        CustomEventArgs someArgs = { 42 };
        SomeHandler()->Trigger(someArgs);
    }

private:
    EventHandler<CustomEventArgs> * m_handler;
};

class Bar : public IDelegate<Foo::CustomEventArgs>
{
public:
    void operator ()(const Foo::CustomEventArgs & args) const
    {
        std::cout << args.i << " bottles of beer" << std::endl;
    }

    void StartListening(const Foo & foo)
    {
        foo.SomeHandler()->AddListener(*this);
    }
};

This code would basically do what I want, but there are still several things I don't quite like.

EventHandler::Trigger() is public

Anyone can call it, which is a serious flaw. Only the owner should be able to call that method. I can see two ways of ensuring this, but I am not happy with any of them:

  • Making the EventHandler(s) hidden from outside Foo. This defeats the purpose though, since it would then require to add as many AddListener/RemoveListener methods.
  • Making Trigger() protected, deriving from EventHandler, then declaring Foo as a friend. This would require to declare a specific class for every single pair of event-handler/class-using-that-event-handler, which is less than ideal.

SomeHandler() returns a pointer

If possible, I think it'd be better to make m_handler an actual member instead of having it somewhere (else) on the heap, and return a reference to it. But I don;t see how to do that without getting plagued with non-const-ness: I want StartListening() to take a const reference.

The question[s]

  1. What would you recommend regarding these two issues?
  2. Do you see other flaws in this design?
\$\endgroup\$

closed as off-topic by Jamal Aug 10 '14 at 0:16

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – Jamal
If this question can be reworded to fit the rules in the help center, please edit the question.

1
\$\begingroup\$

Been a while since I've done much in the way of heavy C++ & I'm sure others will weigh in with their own suggestions, but my two cents:

  1. I think the best solution is the first one you rejected: add Foo::{Add,Remove}Listener. Yep, it's more code but (a) it solves both the issues you observe with the code & (b) the implementation of these methods will be trivial. It's a compromise: better encapsulation in exchange for a little more code.

    If you're really set on your approach, I'm pretty sure you could work around issue #1 by separating EventHandler into two classes (e.g. EventHandler & EventTrigger) & using friends to expose the dirty details, but IMO that's comparatively ugly. This stuff doesn't deserve that kind of hack.

    Along that line of thinking but maybe a little cleaner: have EventTrigger derive from EventHandler (the former providing ::Trigger, the latter ::{Add,Remove}Listener), making m_handler an EventTrigger & having Foo::SomeHandler return a pointer to an EventListener -- effectively "hiding" the Trigger method from the outside world. You're still "leaking" here because you'll need to expose a protected instance variable or method (the list of listeners), but this stuff is simple enough that it shouldn't be completely abhorrent.

  2. Depends what you consider a "flaw" :) Implementors of IDelegate might find the fact the method is const to be annoying (but then you might be making assumptions about implementations of IDelegate that I'm not aware of). Other than that & the issues you observed, things look fine to me.
\$\endgroup\$
  • \$\begingroup\$ Thank you, I'll reconsider the Foo::{Add,Remove}Listener solution with regards to alternatives. What I'm worried about is to see lots of "empty" code clutter in case there are many kinds of events. \$\endgroup\$ – user30189 Oct 2 '13 at 7:22
  • \$\begingroup\$ I like your suggestion of splitting EventTrigger/EventHandler. Sure you can always cast, but at least now the intent is clear. \$\endgroup\$ – user30189 Oct 2 '13 at 7:23
  • \$\begingroup\$ I wondered too about whether the IDelegate method should be const or not, I don't know which is wiser. \$\endgroup\$ – user30189 Oct 2 '13 at 7:41
1
\$\begingroup\$

What Tom Lee said was great (and I'd upvote if I could), but I'd like to add a few misc remarks.

On the constness of the IDelegate method: without more knowledge of what you're trying to implement, I'd vote against it. The reason here is that your delegate reacts to an event, without returning any new computed value (as indicated by the void return type); this would tend to indicate that the only way the delegate could do anything useful would be either by modifying itself or by having "external" side effects. The const restriction forces your delegates to only actively react to an event with external side effects. Of course, removing the const qualifier won't magically forbid you to make side effects, but it might remove the need for them. (Hope that's clear enough.)

On the same topic, I don't see why you intend to pass Foo as a const ref to StartListening? The method modifies the Foo instance, maybe not programatically, but at least semantically: it'd be way more correct IMHO to have a non-const ref given to the StartListening method.

As for trigger / add / remove, it seems to me you wouldn't need a friend declaration if Foo publicly inherited from EventHandler and Trigger were protected: it would publicly expose AddListener and RemoveListener, and you'd be able to call Trigger only from Foo methods, which is what you aim to do if I'm not mistaken? Did I miss something?

Some design remarks concerning Bar::StartListening method. Is there a need for it to have a Foo parameter? Wouldn't it be better to just give it a EventHandler<Foo::CustomEventArgs>? Another thing that bothers me is why you chose to put a registration method in the delegate object at all: you already have a registration method, which is AddListener. By adding StartListening, you make Bar "aware" of Foo (or at least of EventHandler), which it doesn't need: theoretically, Bar should and could work with any EventHandler<CustomEventArgs>, not just with Foo (just like Foo works with any delegate, not just Bar, which it doesn't know). That's why I'd move the registration call out of Bar and make the "link" at an upper level.

My tuppence worth, hope that helps!


EDIT: so, if you want Bar to be aware of Foo, but are reluctant to have Bar hold to a non-const pointer or ref to a Foo instance, I see two imperfect solutions.

The first is to move EventHandler "outside" of Foo: either Foo holds a non-const pointer member (as in your code), or the EventHandler is stored and accessed outside of Foo. Foo is const, Bar can happily register / unregister at will, but this doesn't solve the original "Trigger is public" issue.

Another solution is for Bar to hold a non-const reference to something that is a subset of Foo (interface segregation). For instance, consider this:

class IFoo
{
  public:
    virtual int MyConstMethod1() const = 0;
    virtual float MyConstMethod2(int, int) const = 0;

    virtual void AddListener(const IDelegate<CustomEventArgs> & delegate) = 0;
    virtual void RemoveListener(const IDelegate<CustomEventArgs> & delegate) = 0;
};

class Foo : public IFoo { /* ... */ };

Now Bar may hold a non-const pointer or ref to an IFoo without having access to any of the non-const API of Foo excepted AddListener / RemoveListener, as it only has access to what it's supposed to use. The only issue is that it's a bit verbose.

\$\endgroup\$
  • \$\begingroup\$ I like your point on IDelegate. \$\endgroup\$ – user30189 Oct 2 '13 at 8:39
  • \$\begingroup\$ Foo and Bar are only examples. StartListening is just emphasizing the consequence of having or not a const event handler. For a more realistic example: imagine a Viewer object that reacts to events of a UIControl, which is exposed by a UIControlsContainer. You probably want to have a Register/Unregister pair of functions to call depending on focus for example. I think UIControlsContainer shouldn't have to expose a non const UIControl just because of some event listener somewhere. \$\endgroup\$ – user30189 Oct 2 '13 at 8:47
  • \$\begingroup\$ Yep, OK, I see your point. \$\endgroup\$ – Nicuvëo Oct 2 '13 at 8:52
  • \$\begingroup\$ (continued) If you want to be able to register to and from an object without it exposing a mutable interface, then the only "non-tricky" way to do it is to move the delegate list outside of the object (which then requires trigger to be public, which you wanted to avoid). Another solution would be for your delegate to know only about a "subset" of Foo, via an interface that only exposes what Bar needs: non-const registration methods and const methods: it'd hold a non-const pointer, but not to the whole Foo. \$\endgroup\$ – Nicuvëo Oct 2 '13 at 9:01
  • \$\begingroup\$ I am not sure to understand what you mean. How about you add an example to your answer? \$\endgroup\$ – user30189 Oct 2 '13 at 9:06