Welcome, Guest. Please login or register. Did you miss your activation email?

Author Topic: Peer code review on EventManager  (Read 2118 times)

0 Members and 2 Guests are viewing this topic.

foobarbaz

  • Jr. Member
  • **
  • Posts: 53
    • View Profile
Peer code review on EventManager
« on: February 22, 2014, 04:49:08 am »
So today I revamped my EventManager to use signals. The client code is a lot cleaner I think :D

I haven't finished it yet (still need to remove listeners, more unit tests, etc.), but I was hoping I could get some peer code review. Go ahead and tear it apart :P

Teh codez
https://github.com/tedsta/Fission/blob/master/include/Fission/Core/EventManager.h

The unit test
https://github.com/tedsta/Fission/blob/master/src/Fission/Tests/TestEventManager.cpp

I'm concerned about efficiency, whether or not I should be using RTTI, and code cleanliness. I guess whether or not I'm doing things "right."

Have at me!
Thanks in advance.

zsbzsb

  • Hero Member
  • *****
  • Posts: 1409
  • Active Maintainer of CSFML/SFML.NET
    • View Profile
    • My little corner...
    • Email
Re: Peer code review on EventManager
« Reply #1 on: February 22, 2014, 05:46:52 am »
I'm no expect when to it comes to C++ functions/events/callbacks so to me your code looks fine.

One thing I did notice was your include protection, unless I am totally mistaken you should avoid leading underscores for identifiers in C++.
Motion / MotionNET - Complete video / audio playback for SFML / SFML.NET

NetEXT - An SFML.NET Extension Library based on Thor

Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: Peer code review on EventManager
« Reply #2 on: February 22, 2014, 10:35:41 am »
Instead of manually extracting the hash code from a std::type_info object and using it as std::size_t key in the map, you can directly use std::type_index.

Also, what is Nano? Is there a reason not to use std::function? The following code looks quite complicated, I'm not sure if it's whether the library intends that usage or whether you use it wrong:
template <typename Evt_T, typename T, void (T::*meth_ptr)(const Evt_T&)>
void addListener(T& listener)
{
     ...
    Nano::signal<void (const Evt_T&)>& signal = static_cast<SignalWrapper<Evt_T>*>(mSignals[type].get())->signal;
    signal.template connect<T, meth_ptr>(&listener);
}

But I guess with std::function (or a map of it, if you require signals) it would be simpler. You can have a look at my class thor::EventSystem to see how it might look like -- the complexity in my code is mainly due to the possibility to remove listeners; if you don't need that, it will be much simpler.

In any case, the idea of modern C++ functionals is that you do not have an abstract base class and dynamic allocations in the API, but rather a function object with value semantics.

And yes, the identifier __EVENT_MANAGER_H__ is forbidden. You should prefix it with your library anyway, e.g. FISSION_EVENT_MANAGER_H.
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development:

foobarbaz

  • Jr. Member
  • **
  • Posts: 53
    • View Profile
Re: Peer code review on EventManager
« Reply #3 on: February 22, 2014, 11:42:19 am »
Codeblocks auto generates those header guards. I'll fix them, thanks for the info. I hope I can reconfigure the way codeblocks does it.

Thanks nexus for the std::type_index tip, looks good.

Nano is a simple signal implementation: https://code.google.com/p/nano-signal-slot/

After looking at std::function, it does look like the Nano::signal is overkill. Although, it does make removing listeners really easy. I guess the signal would basically do all the heavy lifting for me? Anyway, I updated the code to use std::function, it does look simpler. But this std::placeholder business confuses me.

Thanks for the input guys.


Nexus

  • SFML Team
  • Hero Member
  • *****
  • Posts: 6287
  • Thor Developer
    • View Profile
    • Bromeon
Re: Peer code review on EventManager
« Reply #4 on: February 22, 2014, 11:57:08 am »
I don't want you to convince from not using Nano, I don't know that library. It's always good to consider alternatives, std::function isn't necessarily the fastest implementation. I was just a bit scared by the verbose code ;)
Zloxx II: action platformer
Thor Library: particle systems, animations, dot products, ...
SFML Game Development: