Shoot the Messenger pt. 2

This is a follow up to Messenger supporting notifications and requests

I’ve written a lightweight (I think) class that acts as a messenger service between classes for both notifications (fire and forget updates to other classes) and requests (a notification sent out that expects a returned value).

Since the last question, I have altered the request system to return an IEnumerable filled with all the results from all of the registered functions. This allows the caller to pick from the results or operate on all of them, although I imagine typical use will be to call .Single() or .First().

I have also removed the default static instance to prevent laziness and poor code practices, and I have removed a pointless cast between Action and Delegate

I’m looking for a general review here on style, usability, best practices, etc.

Here’s the code (.NET Fiddle here)

public class Messenger
{
    /// <summary>
    /// The actions
    /// </summary>
    private Dictionary<Type, Delegate> actions = new Dictionary<Type, Delegate>();

    /// <summary>
    /// The functions
    /// </summary>
    private Dictionary<Type, Collection<Delegate>> functions = new Dictionary<Type, Collection<Delegate>>();

    /// <summary>
    /// Register a function for a request message.
    /// </summary>
    /// <typeparam name="T"> Type of message to receive. </typeparam>
    /// <typeparam name="R"> Type of the r. </typeparam>
    /// <param name="request"> The function that fills the request. </param>
    public void Register<T, R>(Func<T, R> request)
    {
        if (request == null)
        {
            throw new ArgumentNullException("request");
        }

        if (functions.ContainsKey(typeof(T)))
        {
            functions[typeof(T)].Add(request);
        }
        else
        {
            functions.Add(typeof(T), new Collection<Delegate>()
            {
                request
            });
        }
    }

    /// <summary>
    /// Register an action for a message.
    /// </summary>
    /// <typeparam name="T"> Type of message to receive. </typeparam>
    /// <param name="action"> The action that happens when the message is received. </param>
    public void Register<T>(Action<T> action)
    {
        if (action == null)
        {
            throw new ArgumentNullException("action");
        }

        if (actions.ContainsKey(typeof(T)))
        {
            actions[typeof(T)] = (Action<T>)Delegate.Combine(actions[typeof(T)], action);
        }
        else
        {
            actions.Add(typeof(T), action);
        }
    }

    /// <summary>
    /// Send a request.
    /// </summary>
    /// <typeparam name="T"> The type of the parameter of the request. </typeparam>
    /// <typeparam name="R"> The return type of the request. </typeparam>
    /// <param name="parameter"> The parameter. </param>
    /// <returns> The result of the request. </returns>
    public IEnumerable<R> Request<T, R>(T parameter)
    {
        if (functions.ContainsKey(typeof(T)))
        {
            var applicableFunctions = functions[typeof(T)].OfType<Func<T, R>>();

            foreach (var function in applicableFunctions)
            {
                yield return function(parameter);
            }
        }
    }

    /// <summary>
    /// Sends the specified message.
    /// </summary>
    /// <typeparam name="T"> The type of message. </typeparam>
    /// <param name="message"> The message. </param>
    public void Send<T>(T message)
    {
        if (actions.ContainsKey(typeof(T)))
        {
            ((Action<T>)actions[typeof(T)])(message);
        }
    }

    /// <summary>
    /// Unregister a request.
    /// </summary>
    /// <typeparam name="T"> The type of request to unregister. </typeparam>
    /// <typeparam name="R"> The return type of the request. </typeparam>
    /// <param name="request"> The request to unregister. </param>
    public void Unregister<T, R>(Func<T, R> request)
    {
        if (functions.ContainsKey(typeof(T)) && functions[typeof(T)].Contains(request))
        {
            functions[typeof(T)].Remove(request);
        }
    }

    /// <summary>
    /// Unregister an action.
    /// </summary>
    /// <typeparam name="T"> The type of message. </typeparam>
    /// <param name="action"> The action to unregister. </param>
    public void Unregister<T>(Action<T> action)
    {
        if (actions.ContainsKey(typeof(T)))
        {
            actions[typeof(T)] = Delegate.Remove(actions[typeof(T)], action);
        }
    }
}

Example usage:

public class Receiver
{
    public Receiver(Messenger messenger)
    {
        messenger.Register<string>(x =>
            {
                Console.WriteLine(x);
            });

        messenger.Register<string, string>(x =>
            {
                if (x == "hello")
                {
                    return "world";
                }
                return "who are you?";
            });

        messenger.Register<string, string>(x =>
        {
            if (x == "world")
            {
                return "hello";
            }
            return "what are you?";
        });
    }
}

public class Sender
{
    public Sender(Messenger messenger)
    {
        messenger.Send<string>("Hello world!");

        Console.WriteLine("");

        foreach (string result in messenger.Request<string, string>("hello"))
        {
            Console.WriteLine(result);
        }

        Console.WriteLine("");

        foreach (string result in messenger.Request<string, string>("world"))
        {
            Console.WriteLine(result);
        }
    }
}

Good

• You are following the naming guidelines
• The method and parameternames are well choosen and meaningful

Improvable

• XML comments should, if used, be complete e.g /// <typeparam name="R"> Type of the r. </typeparam>

• instead of often calling typeof(T) call it once and reuse it.

• using early returns removes horizontal spacing

• creation of the Collection<Delegate>()

          functions.Add(typeof(T), new Collection<Delegate>()
          {
              request
          });  
  

  this is less readable than this

   functions.Add(type, new Collection<Delegate>() { request });
  

So this

public void Register<T, R>(Func<T, R> request)
{
    if (request == null)
    {
        throw new ArgumentNullException("request");
    }

    if (functions.ContainsKey(typeof(T)))
    {
        functions[typeof(T)].Add(request);
    }
    else
    {
        functions.Add(typeof(T), new Collection<Delegate>()
        {
            request
        });
    }
}  

will become

public void Register<T, R>(Func<T, R> request)
{
    if (request == null)
    {
        throw new ArgumentNullException("request");
    }

    var type = typeof(T);

    if (functions.ContainsKey(type))
    {
        functions[type].Add(request);
        return;
    }

    functions.Add(type, new Collection<Delegate>() { request });

}  

or this

public IEnumerable<R> Request<T, R>(T parameter)
{
    if (functions.ContainsKey(typeof(T)))
    {
        var applicableFunctions = functions[typeof(T)].OfType<Func<T, R>>();

        foreach (var function in applicableFunctions)
        {
            yield return function(parameter);
        }
    }
}  

will become

public IEnumerable<R> Request<T, R>(T parameter)
{
    var type = typeof(T);
    if (!functions.ContainsKey(type))
    {
        return Enumerable.Empty<R>();
    }

    var applicableFunctions = functions[type].OfType<Func<T, R>>();

    foreach (var function in applicableFunctions)
    {
        yield return function(parameter);
    }
}

A couple of small points:

1) Develop to interfaces

2) Signify intent by marking invariants as readonly

/// <summary>
/// The actions
/// </summary>
private readonly IDictionary<Type, Delegate> actions = new Dictionary<Type, Delegate>();

/// <summary>
/// The functions
/// </summary>
private readonly IDictionary<Type, ICollection<Delegate>> functions = new Dictionary<Type, ICollection<Delegate>>();