4
\$\begingroup\$

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).

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

Here's the code (.NET Fiddle with usage):

public class Messenger
{
    /// <summary>
    /// Gets the default messenger.
    /// </summary>
    /// <value> The default messenger. </value>
    public static Messenger Default
    {
        get
        {
            defaultMessenger = defaultMessenger ?? new Messenger();

            return defaultMessenger;
        }
    }

    /// <summary>
    /// The default messenger
    /// </summary>
    private static Messenger defaultMessenger;

    /// <summary>
    /// The actions hooked up to messages
    /// </summary>
    private Dictionary<Type, Delegate> actions = new Dictionary<Type, Delegate>();

    /// <summary>
    /// The functions hooked up to requests
    /// </summary>
    private Dictionary<Type, Delegate> functions = new Dictionary<Type, 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)] = Delegate.Combine(functions[typeof(T)], request);
        }
        else
        {
            functions.Add(typeof(T), 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 R Request<T, R>(T parameter)
    {
        if (functions.ContainsKey(typeof(T)))
        {
            var function = functions[typeof(T)] as Func<T, R>;
            if (function != null)
            {
                return function(parameter);
            }
        }
        return default(R);
    }

    /// <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)] = Delegate.Remove(functions[typeof(T)], request);
        }
    }

    /// <summary>
    /// Unregister a message.
    /// </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)] = (Action<T>)Delegate.Remove(actions[typeof(T)], action);
        }
    }
}

And usage (assuming a Receiver was created before the Sender):

public class Receiver
{
    public Receiver()
    {
        Messenger.Default.Register<string>(x =>
            {
                Console.WriteLine(x);
            });
        Messenger.Default.Register<string, string>(x =>
            {
                if (x == "hello")
                {
                    return "world";
                }
                return "who are you?";
            });
    }
}

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

        Console.WriteLine(Messenger.Default.Request<string, string>("hello"));

        Console.WriteLine(Messenger.Default.Request<string, string>("hi"));
    }
}

Output is:

Hello world!
world
who are you?

Any type can be provided as a message type, string would be an unusual case and typically you would use a custom type for your message, or an enum in smaller use cases.

\$\endgroup\$
  • \$\begingroup\$ Do you know about MVVM Messenger? \$\endgroup\$ – ANeves wants peace for Monica Nov 27 '14 at 17:40
  • 1
    \$\begingroup\$ This is actually based off of MVVMLight's messenger's contract with a few things removed, and the idea of requests added to get around the syntactic hurdle of sending a message class that has a callback parameter. I haven't actually looked at the MVVMLight's messenger implementation to see how they do it, but I rolled my own partly as a learning exercise and also because I didn't like the idea of sending a reference to yourself to the messenger when you register for a message. \$\endgroup\$ – Nick Udell Nov 27 '14 at 18:35
4
\$\begingroup\$
  1. Given that Messenger is a fairly light weight class I don't quite see the point of lazy initialization for the static instance. Why not simply use a static initializer and a readonly field

    public class Messenger
    {
         public static readonly Messenger Default = new Messenger();
    }
    
  2. I would get rid of the Default static instance. It tends to encourage bad practices. It's easy enough to add later when there is a use case where it really can't be solved any other way but having it there just for convenience purposes is a bad idea.

  3. The cast here:

    actions[typeof(T)] = (Action<T>)Delegate.Combine(actions[typeof(T)], action);
    

    is unnecessary since actions uses Delegate as value type.

  4. I'm somewhat dubious about the usefulness being able to register multiple Funcs in an invocation list. The result of calling that invocation list will be the result of the last method being called hence the result will depend on the order of registration which is generally brittle to rely on.

\$\endgroup\$
  • \$\begingroup\$ Excellent points, I had completely forgotten about the issue with multiple registrations. I guess I should either mandate that you can only register one function per request signature, or perhaps return function results as an IEnumerable of their return type by executing all requests. \$\endgroup\$ – Nick Udell Nov 27 '14 at 18:39

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.