4
\$\begingroup\$

I'm working on the communication module of an ecommerce application. This module takes care of handling messages that come from other apps over the wire. The awful switch case is never an option so I decided to look for some pattern. While reading about this topic found that Message Factory and Double Dispatch patterns are the very useful in this scenario. I created my own version following those, but there is one issue I cannot figure out (maybe is ok the way I'm doing it). The things is: as I have one class responsible for handling each message, where should I put the code that register each message handler? In the code I've seen that logic is putted together in some method like in the code, but that will imply one line for each message handler, and maybe there is a shortcut for this. I thought about using reflection to discover Message Handlers at runtime but that would be a performance penalty just to avoid a couple lines of code that I can write I forget about them. Any other idea?

Interface

/// <summary>
/// Represents the entity in charge of handling messages from other applications
/// </summary>
interface ICommunicationService
{
    void RegisterHandler(IMessageHandler handler);

    void ProcessMessage(Message message);
}

Concrete implementation

class CommunicationService : ICommunicationService
{
    private readonly Dictionary<int, IMessageHandler> handlers = new Dictionary<int, IMessageHandler>();

    public void CommunicationService()
    {
        // this is what I want to avoid or improve.
        RegisterHandler(new LoginResponseMessageHandler());
        // ... a line for each handler should go next
    }

    public void RegisterHandler(IMessageHandler handler)
    {
        if (!handlers.ContainsKey(handler.Command))
            handlers.Add(handler.Command, handler);
        else
            handlers[handler.Command] = handler;
    }

    public void ProcessMessage(Message message)
    {
        // Incoming messages that does not have a handler registered will be ignored. This could be or not
        // the desired behavior.
        if (handlers.ContainsKey(message.Command))
            handlers[message.Command].ProcessMessage(message);
    }
}

Message Handlers

interface IMessageHandler
{
    /// <summary>
    /// Gets the identifier of the Command this handler will process.
    /// </summary>
    int Command { get; }

    /// <summary>
    /// When implemented, should contain the logic for handling the requested command.
    /// </summary>
    void ProcessMessage(Message message);
}

Example message handler

class LoginResponseMessageHandler : IMessageHandler
{
    public int MessageId
    {
        get { return 1; } // set the id of the message. this will identify the message across all apps.
    }

    public void ProcessMessage(Message message)
    {
        // here we receive the incomming message and we take care of it...
    }
}

PD: Looking at the code another question came to mind. As you can see I'm using an integer value to identify each message between all apps. Is that a good choice?

\$\endgroup\$

3 Answers 3

4
\$\begingroup\$

In addition to RubberDuck's answer, I'd also propose the following improvements.

I don't really see the need to do this:

if (!handlers.ContainsKey(handler.Command))
    handlers.Add(handler.Command, handler);
else
    handlers[handler.Command] = handler;

You can omit the if/else logic and simply assign the value:

handlers[handler.Command] = handler;

I also would advise to avoid a "negative" check in the if-clause. IMHO this is much easier to understand:

if (handlers.ContainsKey(handler.Command))
    handlers[handler.Command] = handler;
else
    handlers.Add(handler.Command, handler);

But like I said, the whole if/else logic isn't necessary in this case anyway; I'm just using it as an example.

Do not that your code currently doesn't prevent overwrites of existing key/value pairs. You might not want this; if you'd use this (instead of your four line if/else block):

handlers.Add(handler.Command, handler);

... then there'd be an exception if the same command is used more than once as a key.

I also advise against this:

if (handlers.ContainsKey(message.Command))
    handlers[message.Command].ProcessMessage(message);

And instead use TryGetValue to avoid unnecessary lookups:

IMessageHandler handler;
if(handlers.TryGetValue(message.Command, out handler)
{
    handler.ProcessMessage(message);
}
\$\endgroup\$
0
3
\$\begingroup\$

As you can see I'm using an integer value to identify each message between all apps. Is that a good choice?

I don't think it's a good choice. Doing this bakes magic numbers into your code. This is a prime candidate for an Enum.

public enum MessageType
{
    Default,
    Another
    //etc.
}

public MessageType MessageId
{
    get {return MessageType.Default;}
}

This will clarify any code that needs to access the MessageId property. For example, compare these two snippets. One uses a magic number while the other uses the Enum.

If (someMessageHandler.MessageId == 0)

And

If (someMessageHandler.MessageId == MessageType.Default)    

The second one is obviously much clearer in intent.

\$\endgroup\$
3
  • \$\begingroup\$ I agree. I'll change to enum. Thanks. What about the way of registering handlers? \$\endgroup\$
    – rareyesdev
    Commented Sep 6, 2014 at 8:29
  • \$\begingroup\$ I'm not a terribly good programmer. I don't feel like I have enough experience to comment on that, but I don't see any problem with it. \$\endgroup\$
    – RubberDuck
    Commented Sep 6, 2014 at 11:14
  • \$\begingroup\$ OK. Thanks for you answer. I'll definitely move to use enum. +1 \$\endgroup\$
    – rareyesdev
    Commented Sep 6, 2014 at 17:22
2
\$\begingroup\$

Another way to approach this is to let the MessageHandlers recognise the message to be processed. This can work very well in cases where one is using Dependency Injection to create the Communication service.

Say we have

public class Message {
}

public interface IMessageHandler {
    bool Match(Message message);
    void Handle(Message message);
}

public interface ICommunicationService {
    void ProcessMessage(Message message);
}

public class CommunicationService : ICommunicationService {
    private readonly List<IMessageHandler> _handlers;
    public CommunicationService(IEnumerable<IMessageHandler> messageHandlers) {
        _handlers = new List<IMessageHandler>(messageHandlers);
    }
    public void ProcessMessage(Message message) {
        // option 1 - no default handler
        var handler = _handlers.FirstOrDefault(h => h.Match(message));
        if (handler != null) {
            handler.Handle(message);
        }

        // option 2 - there is a default handler that matches any message
        _handlers.First(h => h.Match(message)).Handle(message);
    }
}

Registration
Registration can now happen as part of the configuration. We add new handlers by changing the DI config for the application. No need for code changes to the CommunicationService (or whatever is hosting the communication service which would have previously been calling communicationService.RegisterHandler(...))

Matching
I can argue either way on the int vs enum idea.

  • Enums are safer, easier to read et al. but adding a new message type requires a recompile of the server code to allow it to recognise the new message.
  • Using an int (or a string, or some of other id) is more error prone but allows us to add a new message type simply by adding a new handler - which we are hoping is simply drop a new DLL into the folder and update the config.
In this example, I am kicking for touch and leaving it up to the implementer to decide. We implement matching on the message handler as a function. This allows a few nice things
  1. Use an int? Use an enum? Is the server bothered? Just tell is should it use this handler or not.
  2. If we desire, we can make the handler selection based upon the message content. This allows more flexibility / granularity. Instead of a big handler for MessageType A with lots of options inside depending upon the message content we can have small handlers each responsible for a different scenario. Easier to test and easier to add new handlers as new scenarios arise
  3. We can add a catch-all handler at the end of the list that matches any message. This could be a default action for unrecognised messages.

EDIT
As @agarwaen mentions, a linear check of the handlers may be too slow.
The core of the solution is the self identification of the MessageHandlers and the DI configuration. The Match() test can be replaced with a message identifier which can then be used in a lookup. (Previous comments on enum vs. int still stand but need to use one for example :? )

 public enum MessageType {
    None, 
    Simple,
    NotSoSimple,
    Complex,
    VeryComplex,
    YouAreHavingALaugh
}

public interface IMessageHandler {
    MessageType MessageType { get; }
    void Handle(Message message);
}

public class CommunicationService : ICommunicationService {
    private readonly IDictionary<MessageType, IMessageHandler> _handlers;
    public CommunicationService(IEnumerable<IMessageHandler> messageHandlers) {
        _handlers = messageHandlers.ToDictionary(h => h.MessageType);
    }
    public void ProcessMessage(Message message) {
        IMessageHandler handler;
        if (_handlers.TryGetValue(message.MessageType, out handler)) {
            handler.Handle(message   );    
        }
    }
}
\$\endgroup\$
3
  • \$\begingroup\$ Very interesting implementation. The only limitation is that handling each message can take a while because possible all handlers will try to match against the message. Of course this could work for some scenarios, and your code has very strong points. Thanks for your answer. +1 \$\endgroup\$
    – rareyesdev
    Commented Sep 9, 2014 at 0:46
  • \$\begingroup\$ @agarwaen Your comment on the performance is well taken. In our implementation, the small number (less than 50) of messages and low throughput meant that it was not an issue but in other situations it might be. I will edit to show an alternate version \$\endgroup\$
    – AlanT
    Commented Sep 9, 2014 at 8:35
  • \$\begingroup\$ Nice update. Your answer is helping me to improve my implementation, thanks. \$\endgroup\$
    – rareyesdev
    Commented Sep 9, 2014 at 23:53

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.