HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Generic event class - am I over-complicating it?

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
genericcomplicatingclassoverevent

Problem

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 // std::list
#include // std::find
#include // std::function
#include // std::auto_ptr

template
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
void operator()(Args... args)
{
for (typename std::list::iterator i = events.begin(); i != events.end(); ++i)
{
(*i)(args...);
}
}

private: // Private variables
std::list events;

public:
class Delegate
{
private: // Type declarations
typedef std::function 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
void operator()(Args... args)
{
(*functionPtr)(args...);

Solution

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      // 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
void operator()(Args... args)
{
    for (typename std::list::iterator i = events.begin(); i != events.end(); ++i)
    {
        (*i)(args...);
    }
}


I'd change this to:

template
void operator()(Args&&... args)
{
    for (auto i = events.begin(); i != events.end(); ++i)
    {
        (*i)(std::forward(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.

Code Snippets

private: // Private variables

Delegate (const Func& func) : functionPtr(new Func(func))
{
    /* NOP */
}

#include <memory>     // std::auto_ptr
template<typename... Args>
void operator()(Args... args)
{
    for (typename std::list<Delegate>::iterator i = events.begin(); i != events.end(); ++i)
    {
        (*i)(args...);
    }
}
template<typename... Args>
void operator()(Args&&... args)
{
    for (auto i = events.begin(); i != events.end(); ++i)
    {
        (*i)(std::forward<Args>(args)...);
    }
}
Delegate (const Func& func) : functionPtr(new Func(func))

Context

StackExchange Code Review Q#43752, answer score: 5

Revisions (0)

No revisions yet.