6
\$\begingroup\$

I'm currently writing a game and want to encode everything that happens (GameObject has moved, hp changed, was created/destroyed, got/lost a component, ...) in Events that look like this:

class HpChangedEvent : IEvent
{
    public int GameObjectId;
    public int NewHp;
}

I hope that will make it easier to network the game since every change is encoded as a tuple of value types. Multi-threading is not needed for this code. The event listener can react on these events like this:

class HumanView
{
    private int controlledGameObject;
    public void Update()
    {
         if(Input.ForwardPressed)
         {
             Game.EventModule.QueueEvent(new ForceEvent(Vector3.Forward, controlledGameObject));
         }
    }
}
class PhysicModule
{
    public PhysicModule()
    {
        Game.EventModule.AddDelegate(OnForceEvent);
    }
    void OnForceEvent(ForceEvent e)
    {
        var gameObject = Game.GetGameObjectFromId(e.GameObjectId);
        gameObject.force += e.Value;
    }
}

IEvent.cs

public interface IEvent
{
}

EventDelegate.cs

public delegate void EventDelegate<T>(T e) where T : IEvent;

EventModule.cs

public class EventModule
{
    private class EventDelegateNode
    {
        public object EventDelegate { get; private set; }
        public EventDelegate<IEvent> Callable { get; private set; }

        private EventDelegateNode(object eventDelegate, EventDelegate<IEvent> callable)
        {
            EventDelegate = eventDelegate;
            Callable = callable;
        }

        public static EventDelegateNode Create<T>(EventDelegate<T> eventDelegate) where T : IEvent
        {
            EventDelegate<IEvent> callable = delegate(IEvent e)
            {
                eventDelegate((T)e);
            };
            return new EventDelegateNode(eventDelegate, callable);
        }
    }

    private Dictionary<Type, List<EventDelegateNode>> _EventDelegates = new Dictionary<Type, List<EventDelegateNode>>();
    private Queue<IEvent> _EventQueue = new Queue<IEvent>();

    public void AddDelegate<T>(EventDelegate<T> eventDelegate) where T : IEvent
    {
        if(eventDelegate == null)
        {
            throw new ArgumentNullException("eventDelegate");
        }

        var eventType = typeof(T);
        var eventDelegatesList = default(List<EventDelegateNode>);
        if (!_EventDelegates.TryGetValue(eventType, out eventDelegatesList))
        {
            eventDelegatesList = new List<EventDelegateNode>();
            _EventDelegates.Add(eventType, eventDelegatesList);
        }

        if (!Contains(eventDelegatesList, eventDelegate))
        {
            eventDelegatesList.Add(EventDelegateNode.Create(eventDelegate));
        }
    }
    public void RemoveDelegate<T>(EventDelegate<T> eventDelegate) where T : IEvent
    {
        if (eventDelegate == null)
        {
            throw new ArgumentNullException("eventDelegate");
        }

        var eventType = typeof(T);
        var eventDelegatesList = default(List<EventDelegateNode>);
        if (_EventDelegates.TryGetValue(eventType, out eventDelegatesList))
        {
            eventDelegatesList.RemoveAll(e => Object.ReferenceEquals(e.EventDelegate, eventDelegate));
            if (!eventDelegatesList.Any())
            {
                _EventDelegates.Remove(eventType);
            }
        }
    }

    public void FireEvent(IEvent e)
    {
        if (e == null)
        {
            throw new ArgumentNullException("e");
        }

        var eventDelegatesList = default(List<EventDelegateNode>);
        if (_EventDelegates.TryGetValue(e.GetType(), out eventDelegatesList))
        {
            // use ToArray so the list can be edited while looping
            foreach (var eventListener in eventDelegatesList.ToArray())
            {
                eventListener.Callable(e);
            }
        }

        // typeof(Event) is the wildcard listener
        if (_EventDelegates.TryGetValue(typeof(IEvent), out eventDelegatesList))
        {
            // use ToArray so the list can be edited while looping
            foreach (var eventListener in eventDelegatesList.ToArray())
            {
                eventListener.Callable(e);
            }
        }
    }
    public void QueueEvent(IEvent e)
    {
        if (e == null)
        {
            throw new ArgumentNullException("e");
        }
        _EventQueue.Enqueue(e);
    }

    public void Update()
    {
        // use ToArray so the list can be edited while looping
        foreach (var e in _EventQueue.ToArray())
        {
            // remove event from real queue
            _EventQueue.Dequeue();

            FireEvent(e);
        }
    }

    private static bool Contains<T>(List<EventDelegateNode> eventDelegatesList, EventDelegate<T> eventDelegate) where T : IEvent
    {
        foreach (var item in eventDelegatesList)
        {
            if (Object.ReferenceEquals(item.EventDelegate, eventDelegate))
            {
                return true;
            }
        }
        return false;
    }
}
\$\endgroup\$
  • 2
    \$\begingroup\$ Sorry, I have to ask: why not use the framework's built-in multicast delegate system, event, EventHandler<T> and EventArgs? \$\endgroup\$ – Mathieu Guindon Oct 17 '15 at 19:16
  • \$\begingroup\$ @Mat's Mug I tried that first but I still needed to have a EventModule class where the public event EventHandler<HpChangedEvent> OnHpChange; would live and corresponding public void FireEvent(HpChangedEvent e) {...} otherwise every listener needs to know every sender which would introduce unnecessary coupling. Additionally I like that the event registration looks the same for every Event EventModule.AddDelegate(DoSomething); instead of EventModule.OnEvent1 += OnEvent1Handler;. The only benefit with the EventHandler<> would be that EventArgs can be used for more then one Event. \$\endgroup\$ – prydain Oct 17 '15 at 20:13
6
\$\begingroup\$

Delegates syntax are a little bit too much C#2.0. It's confusing for inexperienced developers and a little bit too ninja in my opinion.

In C# 3 or 4 the Func<> and Action<> where introduced. This remains a matter of opinion but I believe you should use Action<> instead of your delegate.

Your method AddDelegate and RemoveDelegate are not very intuitive. What do I add a delegate to? When will it be executed? Why not name this AddEventHandlerFor and RemoveEventHandlerFor. I know, it looks silly, but look at what it looks like in code :

Game.EventModule.AddEventHandlerFor<HpChangedEvent>(event => /*whatever*/);

Neat, isn't it? (Well, I think it looks neat)

Note that the C# Action<> is retroactive, meaning you could pass a delegate there or a method with the same signature without any problems. So it doesn't make a huge change in your code but I think it'd be better this way.

A detail, but your Dictionary<Type, List<EventDelegateNode>> and Queue<IEvent> members should be marked as read-only, no one should change that instance.

I don't think FireEvent nor Update should be public. What's the point of having a centered event handler module if every listener needs to call FireEvent or Update every one in awhile to see if there's new events they need to be processed? You might want to consider running the Update method on a timed thread or call it when there's a new event that is queued or when there's X events that are in the queue.

That :

// use ToArray so the list can be edited while looping
foreach (var e in _EventQueue.ToArray())
{
    // remove event from real queue
    _EventQueue.Dequeue();

    FireEvent(e);
}

Is weird. _EventQueue.Dequeue() returns the dequeued object. So you don't need to use the foreach and you don't need to copy your whole queue in an array. You can work it like this :

while(_EventQueue.Count > 0)
    FireEvent(_EventQueue.Dequeue());

It makes more sense doesn't it?

This :

// typeof(Event) is the wildcard listener
if (_EventDelegates.TryGetValue(typeof(IEvent), out eventDelegatesList))
{
    // use ToArray so the list can be edited while looping
    foreach (var eventListener in eventDelegatesList.ToArray())
    {
        eventListener.Callable(e);
    }
}

Lacks comments, or explanations, or refactoring, I don't know. I don't understand what is this piece of code supposed to achieve. I suppose I would understand with an example, but in my opinion this is just a flag that this should be explained better (either with code or comments).

Finally, that var eventDelegatesList = default(List<EventDelegateNode>); means null. Are you doing this just to avoid writing List<EventDelegateNode> eventDelegatesList = null? If so, you shouldn't. That's pointless. var is a cool tool but you shouldn't force it. It's just really weird in this case.

Apart from these little details, I think your implementation is quite solid!

\$\endgroup\$
  • \$\begingroup\$ Yeah exactly, thanks :p I was searching for the good word. \$\endgroup\$ – IEatBagels Oct 28 '15 at 15:18
  • 1
    \$\begingroup\$ Thanks for the feedback. I will implement the renaming, Action<T> and readonly as well as the EventQueue suggestions. The EventSystem is used in an environment where the update method is called from the game driver class. The FireEvent method is public to allow time critical events to be handled as fast as possible (e.g. the EventModule.Update gets called 5 times per second and the content loader finished loading a 3D model. If the event was fired without queuing the renderer can use the model as soon as possible). \$\endgroup\$ – prydain Oct 28 '15 at 16:32
  • 1
    \$\begingroup\$ To address the last snippet. If you have a listener that wants to get all events he can simply register for the IEvent type and every time an event is fired all listener for the IEvent type get called. That can be used to log all events or collect statistics about the event system. \$\endgroup\$ – prydain Oct 28 '15 at 16:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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