Dynamic network messaging

I'm building a client and server for a game but wanted a generic messaging system in a shared library that let me focus on application logic and was largely separate form the underlying networking I/O details.

I'm mostly looking for suggestions or possible improvements. Performance on the server is a concern with all the casting. I'm stuck on .Net 3.5 for the client (Unity3D) but if there's decent performance improvements to be found for the server I'd consider splitting the library. I'm also using Protobuf-net for serializing messages being sent across the network. My network protocol uses two 4 byte prefixes, one for the message byte length and the 2nd is an int32 that maps to a dictionary of types so I can deserialize messages to their exact type upon arrival. The typemap is built at startup reflecting through all the message subtypes in the same library.

I'll not post the client/server code because I see those as separate concerns (Network I/O, handshaking, serialization etc can be done a bunch of different ways). Lastly, I've cut out most the threading code to make it easier to read.

My approach breaks down messaging into three simple Message, Request and Response types with the following rules:

• All network messages will derive from the Message class.

• Any message that derives from Request will expect a Response in reply.

• Request and Response are both sub-classes of Message.

Message.cs

public partial class Message
{
    public virtual bool IsResponse { get { return false; } }
    public int id { get; }
    public string description { get; }

    static int idCounter = 0;

    public Message()
    {
        id = idCounter++;
    }

    public Message(string description)
    {
        id = idCounter++;
        this.description = description;
    }
}

Response.cs

```
public class Response : Message
{
public MessageStatus status { get; }
public bool Success { get { return status == MessageStatus.Ok; } }
public ov

Constructor chaining

Calling one constructor from another constructor is called constructor chaining and helps to dry (don't repeat yourself) your code.

For instance the Message class would benefit like so

public partial class Message
{
    public virtual bool IsResponse { get { return false; } }
    public int id { get; }
    public string description { get; }

    static int idCounter = 0;

    public Message()
    {
        id = idCounter++;
    }

    public Message(string description)
        :this()
    {
        this.description = description;
    }
}

and the Response class like so

public class Response : Message
{
    public MessageStatus status { get; }
    public bool Success { get { return status == MessageStatus.Ok; } }
    public override bool IsResponse { get { return true; } }

    public Response(int id, string description)
        :this(id, MessageStatus.Ok, description)
    {
    }

    public Response(int id, MessageStatus status, string description)
    {
        this.id = id;
        this.status = status;
        this.description = description;
    }
}

MessageRouter

By using the null-coalescing operator ?? you can clean the Instance property like so

public static MessageRouter Instance
{
    get
    {
        return instance ?? (instance = new MessageRouter());
    }
}

Using a stateful Singleton prevents the class and its caller's to be tested. Assume you have one test to check that a handler is added successfully and a test to remove a handler. The first one is easy, but for the second one you will to first add a handler and then remove it so you are basically testing 2 different/independent parts of your code but tests should only test 1 part of your code.

So you could say, fine I will just remove the handler which you have added with the first test. This is also bad, because one test should not depend on another test. But you say, hey I can live with that problem here. Then I would need to answer, if you change the order of the calling tests the tests won't assert the same way. If at first the remove handler test is executed it will return false.

So a much better way would be to use an Interface which is the injected to the constructor of the caller's and stored in a private field.

Taking a look at this

public bool AddHandler(Action handler) where T : Message
{
    if (handler == null) { return false; }
    var messageType = typeof(T);
    object outHandler;
    if (handlers.TryGetValue(messageType, out outHandler))
    {
        var typedHandler = outHandler as Action;
        handlers[messageType] = typedHandler + handler;
        return true;
    }
    handlers.Add(messageType, handler);
    return true;
}

I see a few points to mention. First if a passed in argument is null I would expect the method to throw an ArgumentNullException instead of just returning false. Second you really should add some vertical space to group related code together / distinct unrelated code like so

public bool AddHandler(Action handler) where T : Message
{
    if (handler == null) { throw new ArgumentNullException("handler"); }
    // if you use C# 6.0 you can use the nameof operator like so
    // if (handler == null) { throw new ArgumentNullException(nameof(handler)); }
    var messageType = typeof(T);

    object outHandler;
    if (handlers.TryGetValue(messageType, out outHandler))
    {
        var typedHandler = outHandler as Action;
        handlers[messageType] = typedHandler + handler;

        return true;
    }

    handlers.Add(messageType, handler);

    return true;
}

This does not cost you any more memory or decreases execution speed, but make it easier to grasp the logic at first glance which makes it a lot easier to maintain.

public bool RemoveHandler(Action handler) where T : Message
{
    if (handler == null) { return false; }
    var messageType = typeof(T);
    return handlers.Remove(messageType);
}

here the messageType variable is only used to remove the entry of the dictionary which just can be omitted like so

public bool RemoveHandler(Action handler) where T : Message
{
    if (handler == null) { return false; }

    return handlers.Remove(typeof(T));
}

Some of the mentioned points aply to the ResponseRouter too.

StackExchange Code Review Q#96634, answer score: 3