Custom Event System in C#

Question

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;
    }
}

Answer 1

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!