4
\$\begingroup\$

This is a follow up to:

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've extracted two interfaces out of the messenger, IMessenger, that handles sending and receiving messages and IRequester, that handles sending and receiving requests.

IMessenger

/// <summary>
/// Interface for strongly-typed messengers.
/// </summary>
public interface IMessenger
{
    /// <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>
    void Register<T>(Action<T> action);

    /// <summary>
    /// Sends the specified message.
    /// </summary>
    /// <typeparam name="T"> The type of message to send. </typeparam>
    /// <param name="message"> The message to send. </param>
    void Send<T>(T message);

    /// <summary>
    /// Unregister an action.
    /// </summary>
    /// <typeparam name="T"> The type of messag to unregister from. </typeparam>
    /// <param name="action"> The action to unregister. </param>
    void Unregister<T>(Action<T> action);
}

IRequester

/// <summary>
/// Interface for strongly-typed central hubs that support anonymous registry of functions to handle requests.
/// </summary>
public interface IRequester
{
    /// <summary>
    /// Register a function for a request message.
    /// </summary>
    /// <remarks>
    /// Request messages have a return value.
    /// </remarks>
    /// <typeparam name="T"> Type of message to receive. </typeparam>
    /// <typeparam name="R"> Return type of the request. </typeparam>
    /// <param name="request"> The function that fulfils the request. </param>
    void Register<T, R>(Func<T, R> request);

    /// <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>
    IEnumerable<R> Request<T, R>(T parameter);

    /// <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>
    void Unregister<T, R>(Func<T, R> request);
}

I've made the instance variables use interfaces instead of concrete types for more flexibility down the line, and I've cached the typeof(T) operation instead of calling it multiple times.

I've also filled out the comments to better represent what was going on and correctly marked my instance collections as readonly.

Messenger

/// <summary>
/// Strongly-typed messenger that also allows for IoC Service Locator requesting.
/// </summary>
public class Messenger : IMessenger, IRequester
{
    /// <summary>
    /// The actions. These are called when a message is sent.
    /// </summary>
    private readonly IDictionary<Type, Delegate> actions = new Dictionary<Type, Delegate>();

    /// <summary>
    /// The functions. These are called when a request is sent.
    /// </summary>
    private readonly IDictionary<Type, ICollection<Delegate>> functions = new Dictionary<Type, ICollection<Delegate>>();

    /// <summary>
    /// Register a function for a request message.
    /// </summary>
    /// <remarks>
    /// Request messages have a return value.
    /// </remarks>
    /// <typeparam name="T"> Type of message to receive. </typeparam>
    /// <typeparam name="R"> Return type of the request. </typeparam>
    /// <param name="request"> The function that fulfils the request. </param>
    public void Register<T, R>(Func<T, R> request)
    {
        if (request == null)
        {
            throw new ArgumentNullException("request");
        }

        var requestType = typeof(T);

        if (functions.ContainsKey(requestType))
        {
            functions[requestType].Add(request);
        }
        else
        {
            functions.Add(requestType, 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 is executed when the message is received. </param>
    public void Register<T>(Action<T> action)
    {
        if (action == null)
        {
            throw new ArgumentNullException("action");
        }

        var messageType = typeof(T);

        if (actions.ContainsKey(messageType))
        {
            actions[messageType] = Delegate.Combine(actions[messageType], action);
        }
        else
        {
            actions.Add(messageType, action);
        }
    }

    /// <summary>
    /// Send a request.
    /// </summary>
    /// <typeparam name="T">The type of request being sent.</typeparam>
    /// <typeparam name="R">Return type of the request.</typeparam>
    /// <param name="parameter">The parameter for the request.</param>
    /// <returns> A collection of results from the request. </returns>
    public IEnumerable<R> Request<T, R>(T parameter)
    {
        var requestType = typeof(T);

        if (functions.ContainsKey(requestType))
        {
            var applicableFunctions = functions[requestType].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 to send. </param>
    public void Send<T>(T message)
    {
        var messageType = typeof(T);

        if (actions.ContainsKey(messageType))
        {
            ((Action<T>)actions[messageType])(message);
        }
    }

    /// <summary>
    /// Unregister from a request.
    /// </summary>
    /// <typeparam name="T"> The type of request to unregister from. </typeparam>
    /// <typeparam name="R"> The return type of the request to unregister from. </typeparam>
    /// <param name="request"> The request to unregister. </param>
    public void Unregister<T, R>(Func<T, R> request)
    {
        var requestType = typeof(T);

        if (functions.ContainsKey(requestType) && functions[requestType].Contains(request))
        {
            functions[requestType].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)
    {
        var messageType = typeof(T);

        if (actions.ContainsKey(messageType))
        {
            actions[messageType] = (Action<T>)Delegate.Remove(actions[messageType], action);
        }
    }
}

Example Usage

Unchanged from before:

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);
        }
    }
}
\$\endgroup\$
1
\$\begingroup\$
  1. I would implement those interfaces in separate classes. I see no interaction between the two implementations in your Messenger class, and they seem like two separate entities with different purpose. So there should be no reason to mix them into a single class.
  2. Have you considered using interfaces instead of Actions? Something like:

    interface IMessenger
    {
        void Register<T>(IReceiver<T> receiver);
        ...
    }
    
    interface IReceiver<T>
    {
        void Handle(T message);
    }
    

    It requires some extra code to be written to "register" something, but it saves a lot of time debugging stuff, because you will work with strong types, instead of some arbitrary actions. At least in my experience (i used both approaches).

  3. I think any implementation of events aggregator must be thread-safe. Users of your interface will not be able to do the proper synchronization themselves. And your implementation will crash as soon there will be more than one thread in your application. :)

  4. It is usually a good idea to add a constraint to message type. Force your messages to implement some IMessage interface. This way you can a) easily find all the messages, that are currently in use, and b) forbid users of your interface to do this:

    messenger.Send<string>("Hello world!");
    //or
    messenger.Send<int>(13);
    

    Can you guees what those messages represent? I know i can't. :) This is way better in my opinion:

    messenger.Send<WarningMessage>(new WariningMessage("The world is in danger!"));
    //or
    messenger.Send<UserChangedMessage>(new UserChangedMessage { UserId = 13 });
    
\$\endgroup\$
  • \$\begingroup\$ Hi, thanks for the comments! For point 4, I'm strongly against restricting developers from using a tool how they want to. If somebody wants to confuse their maintenance programmers by only sending string messages, I'm not going to stop them. In this, I take after the MVVMLight messenger philosophy. I'm not sure I see the advantages to (2), as far as my code is concerned all it needs is an Action to do something. By requiring an interface that would offer little more than the functionality of Action would that not simply be polluting the namespace? \$\endgroup\$ – Nick Udell Dec 17 '14 at 9:13
  • \$\begingroup\$ @NickUdell, 2) easier to debug (you can see the list of subscribers at any point, instead of a bunch of actions god knows from where) and easier to avoid weird errors (its possible to subscribe "wrong" delegate, and close to impossible to subscribe wrong interface). Apart from that - its a matter of taste, i guess. Merely throwing a suggestion at you.:) \$\endgroup\$ – Nikita B Dec 17 '14 at 10:04
  • \$\begingroup\$ @NickUdell, 4) Fair enough. At least as long as you are not that guy who will have to support or work with such code. :) I was "that guy". Didn't like it. :) \$\endgroup\$ – Nikita B Dec 17 '14 at 10:07
  • \$\begingroup\$ I see your point with 2), I'll try it out and see if I find it worth the increased registration effort, thanks :) \$\endgroup\$ – Nick Udell Dec 17 '14 at 10:10
1
\$\begingroup\$

Nitpicking

I just like nitpicking on your code, because usually there isn't much to critique.

  • Xml documentation

    • should be always complete and spelled correctly
      The type of messag to unregister from here you missed an e

    • IEnumerable<R> Request here I would expect that the returning as IEnumerable<R> is mentioned in the documentation

  • Messenger.Register<T, R>()

    /// <typeparam name="T"> Type of message to receive. </typeparam>  
    

    but you name your variable requestType

  • Messenger.Register<T>()

    here you have the the same

    /// <typeparam name="T"> Type of message to receive. </typeparam>  
    

    but you name your variable messageType

Otherwise your code looks good IMHO.

\$\endgroup\$
  • \$\begingroup\$ There's always a few XML comments that slip through the gaps after a refactor, huh? Thanks for catching them! \$\endgroup\$ – Nick Udell Dec 17 '14 at 9:14

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.