Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

This is my EventHandler class, which is extremely simple and can be used on lambdas and similar:

template<typename Signature, typename...Ax>
class Event
{
private:
    typedef std::function<Signature(Ax...)> Function;
    Function _func;

public:

    void CallFunc(Ax... ax)
    {
        if (_func)
        {
            (_func)(ax...);
        }
    }

    void operator+=(Function func)
    {
        _func = func;
    }

};

I'm looking to find improvements as it's basically as simple as it gets.

Example:

Event<void, int> myEvent;
myEvent += [](int arg) { printf("%i", arg); };

myEvent.CallFunc(15);

Would print out 15.

share|improve this question

2 Answers 2

I don't like that you use operator += to set the function. += suggests that I can append a bunch of functions and they all get called when the event happens, which is not the case. myEvent.setFunction([](int){}); would be better.

It would make sense to take advantage of move semantics. The lambda that gets passed into myEvent is moved into func and then copied into _func, when 2 moves would have been enough:

void operator+=(Function func)
{
    _func = std::move(func);
}

Better yet: Make it a template and perfect forward the argument to have only 1 move:

template<class Fun>
void operator+=(Fun &&func)
{
    _func = std::forward<Fun>(func);
}

You get somewhat worse error messages but most likely better performance.

Same goes for CallFunc, it should take a variadic template argument that gets perfect forwarded into the call for efficiency.

Speaking of efficiency, the flexibility of std::function comes at a price. It is known at compile time what types func can be, so _func should be exactly the same type without type erasure and virtual function calls. You can make that work with a template <class Fun> Event<Fun> make_event(Fun &&fun) style function to get the type of the function and create an Event with the appropriate template parameters, but you lose the ability to specify the function later.

There are extra parenthesis in the call (_func)(ax...);. Maybe it is a style thing, but I would write _func(ax...); instead.

Oh, and printf is not very C++y, but it was just an example.

share|improve this answer

operator += to assign a new handler to the event? It seems quite unconventional to me. += gives the impression that you're appending something, not the case there. Why not simply an assignment?

Furthermore, why bother defining a class at all? You could use std::function directly, combined with a template alias:

template<class RetType, class... Args>
using Event = std::function<RetType(Args...)>;

Then use it much the same way, but with the proper function call operator instead:

// define
Event<void> event = []() {
    std::cout << "Hey!\n";
};

// call the handler
event();
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.