8
\$\begingroup\$

Edit

I've added a second pass at this problem based on the answers that have been posted by other users so far: https://codereview.stackexchange.com/a/221525/75659



Context

Before presenting the code for review, I want to be clear that I am not looking for recommendations on how to avoid having to solve the issue that the code is trying to solve in the first place

This is not production code, this is purely for exploratory purposes, and I would prefer answers that remain within the provided requirements.

With that said, let's suppose I have this contrived interface:

public interface IFoo<T>
{
    event Action<T> ChangeValue;
    event Action<object> ChangeObject;
    event Action ChangeEmpty;

    void InvokeChange(T value);
}

With the following contrived requirements:

  • Calling InvokeChange with a value of type T that Foo was constructed with will invoke the three events

    • ChangeValue is invoked with the given value as type T
    • ChangeObject is invoked with the given value as type object
    • ChangeEmpty is invoked with no arguments
  • Handlers that listen to any of the three events are invoked in the global order they were added, regardless of what event they were added to.

    • Effectively there is only a single event here - some 'change' happened - but 3 event signatures that can be utilized by listeners as needed.
  • The implementation can take any form, but must implement IFoo<T> as it is specified above.

  • The implementation does not need to be thread safe and can allocate memory, but no memory should leak when it is used correctly.

Example 1

The order that event listeners are called should correspond to the order in which they were added, regardless of which event they were added to:

IFoo<int> test = new Foo<int>();
test.ChangeEmpty  += ()  => Console.WriteLine("0) Empty!");
test.ChangeObject += (o) => Console.WriteLine("1) Object: {0}", o);
test.ChangeValue  += (v) => Console.WriteLine("2) Value:  {0}", v);
test.ChangeObject += (o) => Console.WriteLine("3) Object: {0}", o);
test.ChangeEmpty  += ()  => Console.WriteLine("4) Empty!");
test.InvokeChange(123);

Should print the following to the Console:

0) Empty!
1) Object: 123
2) Value:  123
3) Object: 123
4) Empty!

Example 2

Adding/Removing event Listeners should work as expected, while still maintaining the same order:

public static void Main()
{
    IFoo<int> test = new Foo<int>();

    test.ChangeEmpty += EmptyHandler;
    test.ChangeObject += ObjectHandler;
    Console.WriteLine("1) EMPTY, OBJECT");
    test.InvokeChange(1);

    test.ChangeEmpty -= EmptyHandler;
    test.ChangeValue += ValueHandler;
    Console.WriteLine("2) OBJECT, VALUE");
    test.InvokeChange(2);

    test.ChangeObject -= ObjectHandler;
    Console.WriteLine("3) VALUE ");
    test.InvokeChange(3);

    test.ChangeObject += ObjectHandler;
    test.ChangeEmpty += EmptyHandler;
    test.ChangeValue += ValueHandler;
    Console.WriteLine("4) VALUE, OBJECT, EMPTY, VALUE");
    test.InvokeChange(4);

    test.ChangeValue -= ValueHandler;
    test.ChangeValue -= ValueHandler;
    test.ChangeEmpty -= EmptyHandler;
    test.ChangeObject -= ObjectHandler;
    Console.WriteLine("5) <NONE>");
    test.InvokeChange(5);
}

static void EmptyHandler() { Console.WriteLine("  - Empty!"); }
static void ObjectHandler(object val) { Console.WriteLine("  - Object: {0}", val); }
static void ValueHandler(int val) { Console.WriteLine("  - Value: {0}", val); }

Should print the following to the Console:

1) EMPTY, OBJECT
  - Empty!
  - Object: 1
2) OBJECT, VALUE
  - Object: 2
  - Value: 2
3) VALUE 
  - Value: 3
4) VALUE, OBJECT, EMPTY, VALUE
  - Value: 4
  - Object: 4
  - Empty!
  - Value: 4
5) <NONE>

Effectively it should perform as if there is a single invokation list that backs all 3 events.

This code is not required to be particularly performant or thread safe, but 'correct'. In other words, invocation order and frequency is correct regardless of how many times a given handler is added/removed from one of the three events.


Code For Review

With all that in mind, the solution I came up with is:

  • ChangeValue is a standard event backed by an Action<T>
  • ChangeObject and ChangeEmpty are custom event accessors that provide a custom add and remove implementation which are ultimately also backed by ChangeValue.
    • The value of ChangeObject.add and ChangeEmpty.add is wrapped in a lambda 'proxy' of signature Action<T>
    • To ensure that the event accessors are able to also remove the corresponding proxy when in ChangeObject.remove and ChangeEmpty.remove, a dictionary is used for each to track a stack of proxies that have been bound for each value added.
      • add pushes a proxy to the stack and is then added with ChangeValue += proxy
      • remove pops a proxy from the stack and is then removed with ChangeValue -= proxy

Here is the relevant code for one of the events:

public event Action<T> ChangeValue = delegate {};

private Dictionary<Action<object>, List<Action<T>>> ObjectEventProxies
    = new Dictionary<Action<object>, List<Action<T>>>();

public event Action<object> ChangeObject
{
    add
    {
        List<Action<T>> eventProxies;
        if(!ObjectEventProxies.TryGetValue(value, out eventProxies))
        {
            eventProxies = new List<Action<T>>();
            ObjectEventProxies.Add(value, eventProxies);
        }
        Action<T> proxy = (T v) => value.Invoke(v);
        eventProxies.Add(proxy);
        ChangeValue += proxy;
    }
    remove
    {
        List<Action<T>> eventProxies;
        if (ObjectEventProxies.TryGetValue(value, out eventProxies))
        {
            Action<T> proxy = eventProxies[eventProxies.Count - 1];
            eventProxies.RemoveAt(eventProxies.Count - 1);
            ChangeValue -= proxy;
        }
    }
}

And the full code, including the above examples: https://dotnetfiddle.net/u7NGMJ

Question

This code fulfills the given requirements but my question is, can this be simplified? There's a fair bit of boilerplate here for something that feels like it should be simpler to do.


To re-iterate, I am looking specifically for answers that still meet all of the listed requirements, regardless of how contrived or unnecessary they might seem. I should be able to swap out my IFoo<T> implementation for yours and have the output be identical.

I am aware I could just use a single Action<object> if I wanted a single signature to work for all cases. That is not what this question is asking.


Edit

I've added a second pass at this problem based on the answers that have been posted by other users so far: https://codereview.stackexchange.com/a/221525/75659

\$\endgroup\$
  • \$\begingroup\$ You are very specific in what you don't want. But I do have a question about what you do want. You don't want to use a Action<Object>, but you do wrap all your event callers to Action<T>. Is this a requirement or can we use - let's say - 'object' as base class? \$\endgroup\$ – dfhwze Jun 1 at 8:22
  • \$\begingroup\$ For clarity (although I see you already added an answer): What I want is that IFoo<T> is usable exactly as specified, and that any implementation meets the requirements I listed, specifically that the examples I provide have identical output. Everything else is implementation details that do not matter within the context of this question. \$\endgroup\$ – Johannes Jun 2 at 4:38
8
\$\begingroup\$

IMO your events should accommodate the C# standard for events:

  public interface IFoo<T>
  {
    event EventHandler<T> ChangeValue;
    event EventHandler<object> ChangeObject;
    event EventHandler ChangeEmpty;

    void InvokeChange(T value);
  }

The EventHandler delegate takes a source object as argument, which may be useful when consuming the event.


You can modify dfhwze's solution slightly to meet all your requirements to:

  public class Foo<T> : IFoo<T>
  {

    private List<object> handlers = new List<object>();

    void AddHandler(object handler)
    {
      handlers.Add(handler);
    }

    void RemoveHandler(object handler)
    {
      int index = handlers.LastIndexOf(handler);
      if (index >= 0)
        handlers.RemoveAt(index);
    }

    public event EventHandler<T> ChangeValue { add => AddHandler(value); remove => RemoveHandler(value); }
    public event EventHandler<object> ChangeObject { add => AddHandler(value); remove => RemoveHandler(value); }
    public event EventHandler ChangeEmpty { add => AddHandler(value); remove => RemoveHandler(value); }

    public void InvokeChange(T value)
    {
      foreach (object handler in handlers)
      {
        Invoke((dynamic)handler, value);
      }
    }

    public void Invoke(EventHandler handler, T value)
    {
      handler?.Invoke(this, EventArgs.Empty);
    }

    public void Invoke<S>(EventHandler<S> handler, S value)
    {
      handler?.Invoke(this, value);
    }
  }

A List<object> may not be the most effective container, if you have many listeners. In that case you'll have to find another.


Another solution building on your own could be:

  public class Foo<T> : IFoo<T>
  {
    private event EventHandler<T> AllEvents;
    private Dictionary<Delegate, (EventHandler<T> Handler, int Count)> allHandlers = new Dictionary<Delegate, (EventHandler<T>, int)>();

    void AddHandler(Delegate source, Func<EventHandler<T>> handlerFactory)
    {
      if (!allHandlers.TryGetValue(source, out var handler))
      {
        handler = (handlerFactory(), 1);
      }
      else
      {
        handler.Count++;
      }

      allHandlers[source] = handler;
      AllEvents += handler.Handler;
    }

    void RemoveHandler(Delegate source)
    {
      if (allHandlers.TryGetValue(source, out var handler))
      {
        handler.Count--;
        AllEvents -= handler.Handler;
        if (handler.Count <= 0)
          allHandlers.Remove(source);
        else
          allHandlers[source] = handler;
      }
    }

    public event EventHandler<T> ChangeValue { add => AddHandler(value, () => value); remove => RemoveHandler(value); }
    public event EventHandler<object> ChangeObject { add => AddHandler(value, () => (src, v) => value.Invoke(src, v)); remove => RemoveHandler(value); }
    public event EventHandler ChangeEmpty { add => AddHandler(value, () => (src, v) => value.Invoke(src, EventArgs.Empty)); remove => RemoveHandler(value); }

    public void InvokeChange(T value)
    {
      AllEvents?.Invoke(this, value);
    }

    public void Clear()
    {
      allHandlers.Clear();
      AllEvents = null;
    }
  }
\$\endgroup\$
  • \$\begingroup\$ Your point regarding the C# event standard is valid, but this does modify the IFoo<T> interface, which for the sake of this specific question means it's insufficient. I do like the simpler approach of effectively reference counting the proxy \$\endgroup\$ – Johannes Jun 2 at 6:00
6
\$\begingroup\$

no memory should leak when it is used correctly.

Adding and removing a handler results in an empty List<T> lingering in the Dictionary. It seems odd to use a list at all... (I'll come back to that).


dfhwze presents a good alternative solution which loads the complexity in one place. I would be inclined to consider the same, but perform conventional type checks instead of relying on dynamic. A simple change to dfhwze's code could be:

public void InvokeChange(T value)
{
    foreach (var listener in listeners)
    {
        if (listener is Action a)
            a();
        else if (listener is Action<T> aT)
            aT(value);
        else if (listener is Action<object> aObject)
            aObject((object)value);
        else
            throw new InvalidOperationException("<< Suitable exception text here, telling the user to shout at the maintainer >>")
    }
}

This requires no more repetition, and is more explicit about what is going on. It aso avoids dynamic, which makes it more maintainable because all dependencies are statically declared.

Using a sorted dictionary/list, you could put everything in the dictionary, and enumerate its values instead of maintain two collections (the dictionary and generic event). This would reduce redundancy (good), improve add/remove complexity, but increase the overhead of calling the event.


Personally I like the proxy approach, but I couldn't bare to have all that code repeated in the events: you might consider packaging it (and ObjectEventProxies) in a class somewhere so that you can reuse it tidily. That said, most of the code is spent implementing a dictionary which stores an ordered list of values (which is a perfectly generic data structure), so you could just throw one of those together, and that would reduce the amount of repetition (and so fragility) significantly without the effort and risk inherent in trying anything more interesting. This would leave the event handler as more like:

add
{
    Action<T> proxy = /* whatever */;
    WhateverEventProxies.Add(value, proxy);
    ChangeValue += proxy;
}
remove
{
    var proxity = WhateverEventProxies.Remove(value);
    ChangeValue -= proxy;
}

All of the complexity of managing the dictionary is gone, and now the intention of these accessors is clear.

One significant advantage of stuffing everything into another class would be that it would be easier to make thread safe; though, in this case, you could probably just use a thread-safe dictionary with a count, since any event handlers added more than once are equivalent (the ordering doesn't matter).


Since you are stuffing everything in ChangeValue to preserve the order, it's possible to add something with, for example ChangeObject only to remove it with ChangeValue, but since delegates provide exactly one method, there is no problem, and this means you can just use one dictionary (with object as the key) to track all the proxies in a single class.

\$\endgroup\$
  • \$\begingroup\$ For enterprise applications, I find the switch also a better alternative than the dynamic approach. But for the sake of this challenge, I wanted to point out this unorthodox possibility. \$\endgroup\$ – dfhwze Jun 1 at 13:44
  • \$\begingroup\$ Explicitly checking for the type is definitely the best approach for that specifically since (at least within the scope of this question) there are only 3 valid types: Action<T>, Action<object> and Action. \$\endgroup\$ – Johannes Jun 2 at 6:07
  • \$\begingroup\$ I really like the idea of moving all the boilerplate logic to a helper object. I've taken parts of each answer posted so far and added my own second iteration - I've realized that in my specific use case you can handle add type checking in Add/Remove and don't have to worry about that in Invoke. Thank you for taking the time to post your answer, it was very useful :) \$\endgroup\$ – Johannes Jun 2 at 8:37
5
\$\begingroup\$

Review

You wrap all event signatures into Action<T>. I would instead store all event listeners as their common base type Object. This way you don't need to create wrapper delegates.

add
    {
        // ..
        Action<T> proxy = (T v) => value.Invoke(v);
        eventProxies.Add(proxy);
        ChangeValue += proxy;
    }

The proxies seem overkill to me. No need for storing listeners in more than 1 backing collection.

private Dictionary<Action<object>, List<Action<T>>> ObjectEventProxies
    = new Dictionary<Action<object>, List<Action<T>>>();

Proposed Solution

There is an elegant solution that meets all of your contrived requirements which takes advantage of the runtime for finding method overloads using the dynamic keyword.

Store all listeners in a LinkedList (allow for duplicates).

 private LinkedList<object> listeners = new LinkedList<object>();

Register/Unregister event listeners without using boiler-plate code.

        private void Register(object listener)
        {
            if (listener == null) return;
            listeners.AddFirst(listener);
        }

        private void Unregister(object listener)
        {
            if (listener == null) return;
            listeners.Remove(listener);
        }

        public event Action<T> ChangeValue
        {
            add => Register(value);
            remove => Unregister(value);
        }

        public event Action<object> ChangeObject
        {
            add => Register(value);
            remove => Unregister(value);
        }

        public event Action ChangeEmpty
        {
            add => Register(value);
            remove => Unregister(value);
        }

Invoke event listeners using the dynamic keyword. The runtime knows which overload of InvokeChange to call. I have added one overload to take any Object as a fallback to avoid runtime exceptions, but this should be unreachable code. The combination of Reverse(), AddFirst() and Remove() ensures correct behavior. Performance is another thing though ..

        public void InvokeChange(T value)
        {
            foreach (var listener in listeners.Reverse())
            {
                Invoke((dynamic)listener, value);
            }
        }

        // these methods require equivalent method signatures ->
        // -> see comments that Action<T> and Action<object> can be combined!

        private void Invoke(Action<T> action, T value)
        {
            action.Invoke(value);
        }

        private void Invoke(Action<object> action, T value)
        {
            action.Invoke(value);
        }

        private void Invoke(Action action, T value)
        {
            action.Invoke();
        }

        private void Invoke(Object unsupportedEventListenerType, T value)
        {
            // ignore or throw exception ..
        }
\$\endgroup\$
  • 1
    \$\begingroup\$ This solution is not doing exactly what's required, because if you bind the same delegate instance twice or more, it'll only be called once and hence the global call order is somewhat broken. But why he wants to have a possible handler called more times in the same invocation is not clear from the question? \$\endgroup\$ – Henrik Hansen Jun 1 at 10:55
  • 1
    \$\begingroup\$ You can combine Invoke(Action<T> action, T value) and Invoke(Action<object> action, T value) in one: Invoke<S>(Action<S> action, S value) \$\endgroup\$ – Henrik Hansen Jun 1 at 10:58
  • 2
    \$\begingroup\$ @HenrikHansen (1) If he wants the exact same handler registered more than once, the hash set must be replaced with an ordinary list. I overlooked this as a requirement. (2) Nice one! \$\endgroup\$ – dfhwze Jun 1 at 11:14
  • 1
    \$\begingroup\$ @Johannes Thanks for the clarification. I will no longer update my anwer because I feel better answers are available now. \$\endgroup\$ – dfhwze Jun 2 at 7:07
  • 1
    \$\begingroup\$ @Johannes That being said, I had to change it one more time just to meet the requirements. \$\endgroup\$ – dfhwze Jun 2 at 7:42
2
\$\begingroup\$

Thank you for the answers that have been posted so far. Each contributed something that I wanted to incorporate in my second iteration of this problem.

Thank you specifically for solutions by

  • dfhwze - using a single backing collection instead of one for each event
  • Henrik Hansen - using reference counting to avoid needing a proxy List for each handler.
  • VisualMelon - separating proxy management logic as its own object.

I've compiled my own approach that pulls from each of these answers


A Utility for Managing the event proxies

I'm realizing that this is a very specific use case - A single Action<T> event that should also fire as Action<object> and Action depending on what the listener needs - that I could see using in a few areas, and so chose to implement it as a separate utility class.

As was pointed out by Henrik Hansen, C#'s EventHandler is arguably preferable but its use would require changing the signature of the events and therefore the signature of any event handler functions that are Added to them, which in this specific case I am trying to avoid.

My implementation of the Event Proxy Utility object is:

public struct EventProxyContainer<T>
{
    private struct EventProxy
    {
        public Action<T> proxy;
        public int count;
    }
    private Dictionary<object, EventProxy> handlerProxies;

    public Action<T> Add(object handler) { /* See Below */ }

    public Action<T> Remove(object handler) { /* See Below */ }
}

Instead of performing the type check in the Invoke, I chose to handle that in the Add function itself. My intuition is that we will be invoking events more than adding them so this should give a bit of performance benefit.

This also means we can actually avoid having to construct a proxy at all for Action<T> handlers.

Add Implementation

public Action<T> Add(object handler)
{
    if(!(handler is Action<object>) && !(handler is Action)) return (Action<T>)handler;
    handlerProxies = handlerProxies ?? new Dictionary<object, EventProxy>();

    EventProxy entry;
    if(handlerProxies.TryGetValue(handler, out entry))
    {
        entry.count += 1;
        handlerProxies[handler] = entry;
    }
    else
    {
        entry = new EventProxy() { count = 1 };
        if(handler is Action<object>)
            entry.proxy = (v) => ((Action<object>)handler).Invoke(v);
        else if (handler is Action)
            entry.proxy = (v) => ((Action)handler).Invoke();
        handlerProxies.Add(handler, entry);
    }
    return entry.proxy;
}

Remove implementation

Again we early out if the handler is Action<T>

public Action<T> Remove(object handler)
{
    if(!(handler is Action<object>) && !(handler is Action)) return (Action<T>)handler;
    handlerProxies = handlerProxies ?? new Dictionary<object, EventProxy>();

    EventProxy entry;
    if(handlerProxies.TryGetValue(handler, out entry))
    {
        entry.count -= 1;
        if(entry.count == 0) 
            handlerProxies.Remove(handler);
        else
            handlerProxies[handler] = entry;
    }
    return entry.proxy;
}

Foo<T> Implementation

This really cleans up the Foo<T> implementation quite nicely:

public class Foo<T> : IFoo<T>
{
    private EventProxyContainer<T> changeProxy;

    public event Action<T> ChangeValue = delegate {};

    public event Action<object> ChangeObject 
    { 
        add => ChangeValue += changeProxy.Add(value); 
        remove => ChangeValue -= changeProxy.Remove(value); 
    }

    public event Action ChangeEmpty 
    { 
        add  => ChangeValue += changeProxy.Add(value); 
        remove => ChangeValue -= changeProxy.Remove(value);
    }

    public void InvokeChange(T value) 
    {
        ChangeValue.Invoke(value);
    }
}

I like this approach because

  • It still satisfies all the original requirements, and produces the same output as my original examples.
  • It can be retrofitted in any case where you have an event of type Action<T> where you also want event listeners to be able to use it as Action<object> and Action instead.
  • The proxy handling logic is well contained to a single utility object and separate from whatever else might exist in IFoo.
  • Reference counting our proxies allows us to limit one memory allocation per Unique Handler
  • We only construct a proxy for Action<object> and Action handlers - Action<T> handlers are added to the backing event object as normal.
  • Add and Remove whitelist only Action<object> and Action, and return null in all other cases, which event += and event -= handles gracefully.

The updated code with examples can be found as a DotNetFiddle here, and as a gist here.

\$\endgroup\$
  • 1
    \$\begingroup\$ Since Action<T> is your main signature, I can understand why you would convert the others to this one, leaving you no problems whatsoever on invocation. I can live with that :-p \$\endgroup\$ – dfhwze Jun 2 at 8:05
  • \$\begingroup\$ I case you wonder why the downvote... I find you should have accepted one of the other answers instead of your own especially that they all very good. \$\endgroup\$ – t3chb0t Jul 16 at 5:37
  • \$\begingroup\$ @t3chb0t That is completely valid. If I could select multiple answers I would have done so. I chose to combine them and credit each of the original authors at the top of my answer as opposed to having to choose one. I felt each of them had something valid to contribute and not one of them was "better" than the rest. \$\endgroup\$ – Johannes Jul 16 at 18:54

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.