Generic event class - am I over-complicating it?

Question

I'd like some advice about any better or cleaner way to implement this class, if there's any.

I made it using this answer, posted on Stack Overflow, as reference. A lot of problems arose when I switched C function pointers to std::function due to the lack of the equality operator on it, so I ended up implementing a internal class to hold a pointer to the function and make it comparable.

#include <list> // std::list
#include <algorithm> // std::find
#include <functional> // std::function
#include <memory> // std::auto_ptr
template<typename FuncTemplate>
class Event
{
public: // Type declarations
 class Delegate;
public:
 Event() = default;
 ~Event() = default;
 Event& operator+=(const Delegate& func)
 {
 events.push_back(func);
 return *this;
 }
 /**
 * Removes the first occurence of the given delegate from the call queue.
 */
 Event& operator-=(const Delegate& func)
 {
 auto index = std::find(events.begin(), events.end(), func);
 if(index != events.end() )
 {
 events.erase(index);
 }
 return *this;
 }
 /**
 * Fires this event.
 *
 * @param args Arguments to be passed to the called functions. Must have the exact same
 * number of arguments as the given event template.
 */
 template<typename... Args>
 void operator()(Args... args)
 {
 for (typename std::list<Delegate>::iterator i = events.begin(); i != events.end(); ++i)
 {
 (*i)(args...);
 }
 }
private: // Private variables
 std::list<Delegate> events;
public:
 class Delegate
 {
 private: // Type declarations
 typedef std::function<FuncTemplate> Func;
 public:
 Delegate (const Func& func) : functionPtr(new Func(func))
 {
 /* NOP */
 }
 inline bool operator== (const Delegate& other) const
 {
 return (functionPtr.get() == other.functionPtr.get() );
 }
 template<typename... Args>
 void operator()(Args... args)
 {
 (*functionPtr)(args...);
 }
 private:
 std::shared_ptr<Func> functionPtr;
 };
};

This is the code I used to test it:

void prints(const std::string& e)
{
 std::cout << "From prints: " << e << std::endl;
}
int main(int argc, char *argv[])
{
 Event<void(const std::string&)> messageReceived;
 auto printLambda = [](const std::string& e) -> void
 {
 std::cout << "From print lambda: " << e << std::endl;
 };
 Event<void(const std::string&)>::Delegate printDelegate1(printLambda); // Testing lambda
 Event<void(const std::string&)>::Delegate printDelegate2(prints); // Testing standard functions
 Event<void(const std::string&)>::Delegate printDelegate3(printLambda); // Testing lamda on another delegate instance
 Event<void(const std::string&)>::Delegate printDelegate4(printDelegate1); // Testing cloning constructor
 Event<void(const std::string&)>::Delegate printDelegate5 = printDelegate1; // Testing assignation
 std::cout << "Is Delegate 1 = Delegate 1? " << (printDelegate1 == printDelegate1) << std::endl;
 std::cout << "Is Delegate 1 = Delegate 2? " << (printDelegate1 == printDelegate2) << std::endl;
 std::cout << "Is Delegate 1 = Delegate 3? " << (printDelegate1 == printDelegate3) << std::endl;
 std::cout << "Is Delegate 1 = Delegate 4? " << (printDelegate1 == printDelegate4) << std::endl;
 std::cout << "Is Delegate 1 = Delegate 5? " << (printDelegate1 == printDelegate5) << std::endl;
 std::cout << std::endl << std::endl;
 messageReceived += printDelegate1;
 messageReceived += printDelegate2;
 messageReceived += printDelegate3;
 messageReceived += printDelegate4;
 messageReceived += printDelegate5;
 messageReceived("This should be printed five times");
 std::cout << std::endl;
 messageReceived -= printDelegate1;
 messageReceived -= printDelegate2;
 messageReceived -= printDelegate3;
 messageReceived("This should be printed two times");
 std::cout << std::endl;
 messageReceived -= printDelegate4;
 messageReceived -= printDelegate5;
 messageReceived("Whoops! Something is wrong");
 return 0;
}

Answer 1

Before looking at the code, and just having a quick read-through, the first thing that sticks out to me are the comments:

private: // Private variables
Delegate (const Func& func) : functionPtr(new Func(func))
{
 /* NOP */
}
#include <memory> // std::auto_ptr

The first is obvious, the second is slightly misleading, and the third is completely wrong (I know you meant std::shared_ptr, but having an incorrect comment is worse than no comment at all).

Your operator() should probably be using perfect forwarding instead of passing a copy of all the arguments. Also, since you're already using lambdas and variadic templates, you might as well take advantage of auto and/or range based for loops:

template<typename... Args>
void operator()(Args... args)
{
 for (typename std::list<Delegate>::iterator i = events.begin(); i != events.end(); ++i)
 {
 (*i)(args...);
 }
}

I'd change this to:

template<typename... Args>
void operator()(Args&&... args)
{
 for (auto i = events.begin(); i != events.end(); ++i)
 {
 (*i)(std::forward<Args>(args)...);
 }
}

Similarly for the operator() in Delegate.

Instead of using new in:

Delegate (const Func& func) : functionPtr(new Func(func))

You should prefer std::make_shared, as it is potentially slightly more efficient, and good practice to get into, as if you ever use new multiple times before a sequence point, there can be memory leaks if (say) the second new throws before the shared_ptr is fully constructed.

• user15111: I wouldn't have noticed the std::auto_ptr comment had you not mentioned it. I use the first comment you mentioned as labels, since the code folding on Sublime 3 doesn't hide them (the Private part is redundant, though). I made the changes you suggested, and I might make a benchmark later to see the differences. Anyway, thanks for your feedback.
• Yuushi: @RaphaelC. Honestly, unless you're passing a number of really expensive parameters, you're unlikely to see much of a difference here. It's (potentially) a construction + move (which for a lot of classes is just swapping pointers) instead of a construction + copy construction.
• Mat: You can cut down that for loop still with a range-based for.