HiveBrain v1.2.0
Get Started
← Back to all entries
principlecsharpMinor

An approach to the network messaging service

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
theapproachmessagingservicenetwork

Problem

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

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

    void ProcessMessage(Message message);
}


Concrete implementation

```
class CommunicationService : ICommunicationService
{
private readonly Dictionary handlers = new Dictionary();

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 n

Solution

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);
}

Code Snippets

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

Context

StackExchange Code Review Q#62078, answer score: 4

Revisions (0)

No revisions yet.