views:

137

answers:

4

As I was looking through SO I came across a question about handling multiple message types. My concern is - how do I load such a message in a neat way? I decided to have a separate class with a method which loads one message each time it's invoked. This method should create a new instance of a concrete message type (say AlphaMessage, BetaMessage, GammaMessage, etc.) and return it as a Message.

class MessageLoader
{
    public Message Load()
    {
        // ...
    }
}

The code inside the method is something which looks really awful to me and I would very much like to refactor it/get rid of it:

Message msg = Message.Load(...); // load yourself from whatever source
if (msg.Type == MessageType.Alpha) return new AlphaMessage(msg);
if (msg.Type == MessageType.Beta) return new BetaMessage(msg);
// ...

In fact, if the whole design looks just too messy and you guys have a better solution, I'm ready to restructure the whole thing.

If my description is too chaotic, please let me know what it's missing and I shall edit the question. Thank you all.

Edit: What I don't like about this code is that I have to create an instance of a Message (cause it knows how to load itself) and then have to decorate it with a concrete message type (cause decorators know how to interpret msg's Data property). Maybe this will make the question slightly more clear.

+1  A: 

The next level of abstraction is to make Message discovery and instantiation dynamic. This is often accomplished by associating a string name with each Message or by using the name of the class as an identifier. You can use Reflection to discover available Message types, store them in a Dictionary and provide instantiation by name. This can be further extended to bring in Messages from dynamically loaded 'plugin' assemblies, with appropriate meta-data and interfaces to allow for loosely coupled composition between different Messages and Message Consumers. Once you get to that level, I recommend looking into frameworks like MEF which automate the discovery and instance injection process.

For your simple application, I think your approach is already quite clean. A series of if statements or a switch works just fine and is easy to understand/maintain, as long as you have a relatively small and stable set of cases.


Summarizing the further discussion in the comments:

The main design concern creating uneasiness was the fact that the different specific messages inherited from Message and yet a base Message had to be instantiated before the more specific messages could perform further analysis. This muddied up whether the Message is intended to contain raw information or to act as a base type for interpreted messages. A better design is to separate the RawMessage functionality into its own class, clearly separating concerns and resolving the feeling of 'instantiating twice'.

As for refactoring with DTOs and a mapper class:

I actually prefer your approach for an app-specific message encoding/decoding. If I want to track down why FactoryTakenOverByRobotsMessage contains invalid data, it makes sense to me that the parser method for the message is contained with the decoded data for the message. Where things get more dicey if when you want to support different encodings, as now you start wanting to specify the DTO declaratively (such as with attributes) and allow your different transport layers to decide how to serialize/deserialize. In most cases where I'm using your pattern, however, it's for a very app-specific case, with often somewhat inconsistent message formats and various proprietary encodings that don't map well in any automatic way. I can always still use the declarative encoding in parallel with the proprietary, in-class encoding and do things like serialize my messages to XML for debugging purposes.

Dan Bryant
So each message actually has a string type id, but I encapsulated it into sort of a 'string enum', probably just to have intellisense. I like the suggestion of using a dictionary to instantiate a concrete message type, but as you said, since this is a simple application, I'm OK with the if tree. I just twitch when I see that each message has to be instantiated twice, in a way. Does that seem fine?
lukem00
Since the second instantiation is a decorator, which adds interpretive functionality, I think that's fine. Basically you're just crunching the raw message through additional processing to extract more useful information. The main thing that's awkward is that you then return it as a Message again, so you end up checking Message types again somewhere else. It might make more sense if the various Message details don't descend from Message and Message is renamed to something like RawMessage.
Dan Bryant
That is exactly what I'm doing! "Interpretive functionality" - really well said. It's so much easier to actually express the whole idea in C# than in plain English, but that's just a side note. What do you mean by not descending from Message? Btw. What I got is a sort of queue where I keep all the messages which need to be processed, that's why I decided it's better to return them as a Message instead of just an object. I know that in messaging a common approach is to have type queues, but I need to process them in FIFO order, so I thought one is better. Sorry for a long-ish comment
lukem00
You really have two different concepts here. The first is the raw message, whereas the second is an interpreted message. They are really two different kinds of object and the interpreted message isn't necessarily a 'kind' of raw message. An interpreted message might include a reference to the raw message that it interpreted, which may ease the twitching feeling that the object is somehow being 'instantiated twice'.
Dan Bryant
"ConcreteMessage is a RawMessage" - just a slight change in name and the whole sentence really sounds ackward now. Could it be another case where inheritance is a bit forced and composition would actually make a better solution? Applying the idea might mean turning some of the code upside down, but it sounds reasonable enough for me to try it. Anything to stop the twitching ;) Thank you for all your insight!
lukem00
Any chance you could edit your answer so that it includes a summary of what we discussed? That way I could mark it as accepted - sorry, I'm probably being a purist here. Also, I tried following your suggestions and I separated messages from their payload - each interpreted message references a raw message instead of inheriting from it. Now as I look at it from a fresh perspective, I'm starting to wonder whether I shouldn't actually go a step further and turn those interpreted messages into DTOs and have a separate mapper fill them with data from raw messages. What do you reckon?
lukem00
Obviously your answer generated a bunch of further questions which could easily lead to a whole new discussion on DTOs and serialization. I guess I've already taken a lot of your time though, so I'm accepting this one as an answer and thank you for all your insight.
lukem00
A: 

With C# you'll probably need something like what you've written because C# is a strongly-typed language. Basically, you have to get the concrete classes somewhere in your code.

John at CashCommons
A: 

What you have looks fine. It's unambiguous. If your AlphaMessage and BetaMessage objects are children of Message then instead of creating a new object, just return the object casted.

if (msg.Type == MessageType.Alpha) return msg as AlphaMessage;
else if (msg.Type == MessageType.Beta) return msg as BetaMessage;

Consumer code will have to handle the case where the cast fails (as returns null).

SnOrfus
Well, AlphaMessage and BetaMessage are children of Message, but Message is not an abstract class - Message.Load() actually creates instances of that class, which later have to be "turned" into concrete message types, with specific properties. Can I really still downcast it (instead of wrapping)? I'm slightly struggling with putting this into words here, sorry for that.
lukem00
+2  A: 

I agree with CkH in that Factory pattern will solve it. I wrote a silly example as a proof of concept. Not meant to show good class design, just that a simple Factory pattern works. Even if you are using multiple message types and handlers, you should only need to modify this pattern slightly.

class Class12
{
    public static void Main()
    {
        Message m = new Message(1, "Hello world");
        IMessageHandler msgHandler = Factory.GetMessageHandler(m.MessageType);
        msgHandler.HandleMessage(m);

        Message m2 = new Message(2, "Adios world");
        IMessageHandler msgHandler2 = Factory.GetMessageHandler(m2.MessageType);
        msgHandler2.HandleMessage(m2);
    }
}
public class Factory
{
    public static IMessageHandler GetMessageHandler(int msgType)
    {
        IMessageHandler msgHandler = null;
        switch(msgType)
        {
            case 1: 
                msgHandler = new MessageHandler1();
                break;
            case 2: 
                msgHandler = new MessageHandler2();
                break;
            default: 
                msgHandler = new MessageHandler1();
                break;
        }
        return msgHandler;
    }
}
public class Message
{
    public int MessageType { get; set; }
    public string AMessage { get; set; }

    public Message(int messageType, string message)
    {
        this.MessageType = messageType;
        this.AMessage = message;
    }
}
public interface IMessageHandler
{
    void HandleMessage(Message m);
}
class MessageHandler1 : IMessageHandler
{

    #region IMessageHandler Members


    public void HandleMessage(Message m)
    {
        string message = m.AMessage;
        Console.WriteLine(message);
    }

    #endregion
}
class MessageHandler2 : IMessageHandler
{

    #region IMessageHandler Members


    public void HandleMessage(Message m)
    {
        string message = m.AMessage;
        Console.WriteLine("Hey there " + message);
    }

    #endregion
}
P.Brian.Mackey
That's a nice example, thanks. See, when it comes to consuming messages I actually do have a dispatch class quite similar to your factory with handlers. What I'm trying to achieve by wrapping messages is, as Dan Bryant nicely described, adding interpretive functionality. So I got: 1) bottom layer - messages with headers and raw payload 2) middle layer - decorated messages which are actually some concrete documents 3) top layer - a set of handlers which process each document in a different way
lukem00