patterncppMinor
Generic event class - am I over-complicating it?
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
```
#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...);
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:
The first is obvious, the second is slightly misleading, and the third is completely wrong (I know you meant
Your
I'd change this to:
Similarly for the
Instead of using
You should prefer
before a sequence point, there can be memory leaks if (say) the second
private: // Private variables
Delegate (const Func& func) : functionPtr(new Func(func))
{
/* NOP */
}
#include // std::auto_ptrThe 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 timesbefore 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_ptrtemplate<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.