6
\$\begingroup\$

I'm using my own Action-based Event Manager for a while and looking for ways to improve it.

Mainly it's used in game development, where excessive garbage generation can lead to severe consequences. That's why I'm trying to avoid any boxing which normally exists in such managers.

There are different event types (dozens, but not hundreds), each event type holds several subscribers. Subscriptions happen not often, but some events can fire hundreds of times per second (it won't actually, but i'm considering the possibility while creating my manager).

There is my previous version which is works just fine, but asks for improvement:

internal class EventManager: IEventManager
{
    private readonly Dictionary<int, Action<object>?> events = new Dictionary<int, Action<object>?>();

    public virtual void Subscribe(int incidentId, Action<object> action)
    {
        if (events.ContainsKey(incidentId)) events[incidentId] += action;
        else events.Add(incidentId, action);
    }

    public virtual void Unsubscribe(int incidentId, Action<object> action)
    {
        if (!events.ContainsKey(incidentId)) return;
        events[incidentId] -= action;
    }


    public virtual void Trigger(int incidentId, object args)
    {
        if (!events.ContainsKey(incidentId)) return;
        events[incidentId]?.Invoke(args);
    }
}

// Sender:
eventManager.Trigger(1, "1234");  
// Receiver:
eventManager.Subscribe(1, a => Console.WriteLine($"Received string length: {(a as string)?.Length}"));

I'm actually wrapping object inside convenient struct, but it doesn't matter. Sender is boxing some data to object, receiver is unboxing it. That's behavior I would like to avoid. Creation of an object each time event is fired can be painful.

There is another version of event manager:

internal class EventManager: IEventManager
{
    private readonly Dictionary<int, Dictionary<Type, Delegate>> events = new Dictionary<int, Dictionary<Type, Delegate>>();

    public virtual void Subscribe<T>(int incidentId, Action<T> action)
    {
        if (!events.ContainsKey(incidentId))
            events[incidentId] = new Dictionary<Type, Delegate> {[typeof(T)] = action};

        else if (!events[incidentId].ContainsKey(typeof(T)))
            events[incidentId][typeof(T)] = action;

        else 
            events[incidentId][typeof(T)] = Delegate.Combine(events[incidentId][typeof(T)], action);
    }

    public virtual void Unsubscribe<T>(int incidentId, Action<T> action)
    {
        if (!events.ContainsKey(incidentId) ||
            !events[incidentId].ContainsKey(typeof(T))) return;

        events[incidentId][typeof(T)] = Delegate.Remove(events[incidentId][typeof(T)], action);
    }


    public virtual void Trigger<T>(int incidentId, T args)
    {
        if (!events.ContainsKey(incidentId) ||
            !events[incidentId].ContainsKey(typeof(T))) return;

        var genericEvent = events[incidentId][typeof(T)] as Action<T>;
        genericEvent?.Invoke(args);
    }
}

// ...

// Sender:
eventManager.Trigger(1, "1234");
// Receiver:
eventManager.Subscribe<string>(1, a => Console.WriteLine($"Received string length: {a.Length}"));

That's much better! Events are still mapped with Ids, senders can send any data and subscribers would get it without need of unboxing. Even conflicting data types between sender and receiver are handled: receiver wouldn't get incorrect message type (in previous version I'd have to add null and default values handling).

But usage of generics leads to another collection of Type which stores generic Actions. I cannot just declare something like Dictionary<int, Action<T>>.

Dictionary inside a dictionary?.. Is it normal? Are there hidden traps somewhere?

Or I'm missing some simple pattern which would allow to pass data via events and get rid of object boxing and support different Ids for different event types?

P.S. Subscriptions are rare, so I'm not necessarily looking into micro-optimizations here. Event firing, on the other hand, could be extremely often.

UPDATE: Clarification: I'd like to receive feedback on second implementation, potential improvements and possible problems in future. First example is just "standard" version: I'm wondering if I'd better to stick with it or use generics magic with second example.

\$\endgroup\$
3
\$\begingroup\$

I don't see any glaring issues with the generic EventManager. Yes Dictionary of Dictionary does happen and is sometimes needed.

One thing to consider is this implementation violates the Liskov substitution principle of SOLID. Usually in a non generic version the code would take the object and do "as" their type and check if not null. While the that wouldn't be an option anymore with the generic. What if the handler wanted to listen to all events for an Id? with the non generic it would have that option. With the generic it couldn't unless it knew all the types that got register, not likely. Also with program growing might start with something like

public class FeatureEvent
{
    public virtual string Title => "Original Event";
}

then in phase 2 or 3 down the road need to expand to add more data.

public class ExtraFeatureEvent : FeatureEvent
{
    public override string Title => "Better Event";
    public DateTime UseAfter { get; set; } = DateTime.Now;
}

public class ExtraFeature2Event : FeatureEvent
{
    public override string Title => "Even Better Event";
    public Guid Id { get; set; } = Guid.NewGuid();
}

Since these both come from FeatureEvent but would be sending data as a different type only the specific types events would get triggered and not the handlers listening for the base event.

You could expand the trigger to handle and check If with IsInstanceOf but then you have to handle casting the type and gets a bit more complex. Only you know if this is something that out weights the casting but it something I see a lot of people forget about when they switch to a generic scheme like this.

Also a side note you might want to constrain the event data to come from a base abstract event class to make it clear this is event data and not send over bunch of data. Like MS does for their events that event data comes from a class that is based on EventArgs.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ That's a lot of things to consider, thanks! "What if the handler wanted to listen to all events for an Id?" - the opposite is also correct, isn't it? If a subscriber expects bool value it would be strange to receive something "0.001f" or "System.Collections.Generic.List". What it supposed to do with it? I see the option to send default data to all subscribers of event if relevant data is not available, but this doesn't feel right. Right? \$\endgroup\$ – Xamtouo Jan 24 at 8:01
  • 2
    \$\begingroup\$ That's the trade off. If as a consume I know I'm getting an object then its up to the caller to cast it. What you are building is most commonly called an Event Aggregator. If you google it you will find lots of information on it and some mvvm projects like prism and Caliburn.Micro and you could check out their source code to see if how others have done it in the past. \$\endgroup\$ – CharlesNRice Jan 24 at 14:49
4
\$\begingroup\$

Generic is your call, as using generic would avoid casting objects.

Would this version work with you ? :

internal class EventManager<T>
{
    private readonly Dictionary<int, Action<T>> events = new Dictionary<int, Action<T>>();

    public virtual void Subscribe(int incidentId, Action<T> action)
    {
        if (events.ContainsKey(incidentId))
        {
            events[incidentId] = action;
        }
        else
        {
            events.Add(incidentId, action);
        }
    }

    public virtual void Unsubscribe(int incidentId, Action<T> action)
    {
        if (events.ContainsKey(incidentId))
        {
            events[incidentId] -= action;
        }
    }

    public virtual void Trigger(int incidentId, T args)
    {
        if (events.ContainsKey(incidentId))
        {
            events[incidentId]?.Invoke(args);
        }
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Nice and straightforward solution. I've considered it: the problem is that for each argument type I want to send I'd have to create new event manager... But there is no actual reason *not to create 5-6 event managers instead of one, just convenience. Although there would be a problem if some event will require triggering all its subscribers regardless of argument type (send default data to all subscribers of event). \$\endgroup\$ – Xamtouo Jan 24 at 7:52
  • \$\begingroup\$ @Xamtouo if your logic required initiating one instance and use it for concurrent events, then it'll be easy to just make it a singleton class. if some event requires triggering all its subscribers, then you would need to create another method to handle the default, and just expose it. As, the dictionary has the events already, you can do whatever you need. If you see there is a certain logic needs a concrete handler, then implement the handler and call it back from this instance. \$\endgroup\$ – iSR5 Jan 24 at 9:54
2
\$\begingroup\$

Do you necessarily want to have one Event-Manager only ?

If you would have EventManager<t>, instead of 3 generic methods, you would have one Eventmanager by Type. It's already resolved at compile time, which dictionary to access.

Today, Trigger<int> and Trigger<string> put their things in the same dictionary, which you need to subdivide by type.

If you have EventManager<T>, a call to Trigger would just insert in the dictionary, other Types would have other dictionaries.

Something else:

If you have influence on how your incidentID is used, you could use a little registry for this, and generate continuous numbers, you just have to define, where this number is used the first time. Doing so, you can safe the top-dictionary also, it would be just a full-length indexed array - very easy to access, very performant. As a side effect you can also attach a string, to the incidentID, this makes debugging messages and exceptions more readable.

| improve this answer | |
\$\endgroup\$

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.