Many Magic Numbers

Question

I have a project where I transform Messages from one format into another. The messages are identified by an id. I have a MappingTable with the id in the one format and the corresponding id in the other format.

For example: 2000 = 153

The id's are integers

I add all those entrys via:

this.AddMappingEntry(2400, 2005, true, false);

This method adds a mapping entry into a mapping list. I can filter the list via linq, so that I can find the correct mapping if I receive a message with value 2400.

Since the project is launched it is grown quite a bit which lead to many refactorings and to many id's with a special behavior. I have to control if a message is of a special type.

if(message.id == 153)
{
    //special behavior
}

How should I handle this most elegant ? Should I use many constants which describe the message type or is there another more elegant way ?

EDIT:

I reword the question. I have Head records and sub records. But the Id's of those records are used throughout the Code. Since there are about 200 different Id's I was wondering what I should do with those magic numbers. The Tool that I am writing is a converter.

The structure is as follows

     +------+         +-------------+      +---------+
     | DAL  +-------> |  CONVERTER  +----> | WRITER  |
     +------+         +-------------+      +---------+

The Converter classes look roughly like this

    +------------+
    |BaseClass   |
    +-----+------+
          ^
          |
    +-----+------+
    |BaseRecord  +^-------------+----------------------+
    +------+-----+              |                      |
           ^                    |                      |
           |                    |                      |
    +------+-----+      +-------+--------+     +-------+--------+
    | HeadRecord |      |   RecordType1  |     |   RecordType2  |
    +------------+      +----------------+     +----------------+

The BaseRecord extends the Baseclass and all other classes extend the BaseRecord. In total I have 5 Record types with a recordId 1-5. Inside this record are several subrecordId's (~50) which are only used to identify the Records behind the writer respectively after the writing process.

The problem is that some records read some values from different fields then others which leads to some special cases where I need to identify the record.

This leads to the Problem: I have many magic numbers which nobody knows what they are for, if I use them in my classes and the converter. How do I avoid those magic numbers ? I am afreid that my classes are filled with 50+ const values if I use those. Is there a way to avoid magic numbers and a big bunch of const values ? What is the correct refactoring for this ?

Answer 1

If you can generalize special behavior as a collection of actions, you should be able to do it like this:

private static readonly IDictionary<int,Action> messageBehavior =
    new Dictionary<int,Action> {
        {153, () => { Console.WriteLine("Special action"); } },
        {154, () => { Console.WriteLine("Another special action"); } }
    };

Now you can fetch the action from the dictionary by message ID, and run it if it is available:

Action special;
if (messageBehavior.TryGetValue(message.id, out special)) {
    special();
}

If actions require some special context in which they run, for example, the message that has triggered the action, you can use Action<Message>, and pass message to it:

private static readonly IDictionary<int,Action<MyMessageType>> messageBehavior =
    new Dictionary<int,Action> {
        {153, (m) => { Console.WriteLine("Special action; message {0}", m); } },
        {154, (m) => { Console.WriteLine("Another special action ({0})", m); } }
    };
...
Action<MyMessageType> special;
if (messageBehavior.TryGetValue(message.id, out special)) {
    special(message);
}

Answer 2

This is a place where the Factory design pattern can come in handy.

Define an interface for a class that will know how to deal with a single special behavior:

public interface IMessageHandler
{
    IMessage Transform(IMessage message);
}

Then you will have it's different implementations. For example:

public class DefaultMessageHandler
{

    IMessage Transform(IMessage message)
    {
        return message;
    }
}

public class BehaviorXMessageHandler
{

    IMessage Transform(IMessage message)
    {
        message.SomeProperty = "hello world";
        return message;
    }
}

Then you have your factory:

public interface IMessageHandlerFactory
{
    IMessageHandler GetHandler(int messageCode);
}

A possible implementation for the factory will be to use switch-case but I think you should use Dependency Injection:

In the following implementation you pass both the mapping and a default behavior for all the non-special cases. I think this is a elegant way to map only what you need and to keep it organaized

public class MappingMessageHandlerFactory : IMessageHandlerFactory
{
    public MappingMessageHandlerFactory(Dictionary<int,IMessageHandler> mapping, IMessageHandler defaultBehavior)
    {
        Mapping = mapping;
        DefaultBehavior = defaultBehavior;
    }

    public IMessageHandler GetHandler(int messageCode)
    {
        IMessageHandler output = DefaultBehavior;
        Mapping.TryGetValue(messageCode, out output);

        return output;
    }

    public Dictionary<int,IMessageHandler> Mapping {get; set;}
    public IMessageHandler DefaultBehavior {get;set;}
}

For the Factory to receive the mapping you can initialize it yourself or use one of the many IoC Containers out there such as Castle Windsor, Ninject, Unity, etc.

Some of these containers even have for you generic implementations for the factories and all you need to provide is the interface. In Castle Windsor it is called TypedFactoryFacility

Whatever you choose, after going through the structure above the code of your service should just look something like:

public IMessage ProcessMessage(IMessage message)
{
    var handler = _messageHandlerFactory.GetHandler(message.Code);
    return handler.Transform(message);
}

---

Update after comment:

But in the conversion process I will have to use the magic numbers in some other places which is bad because I have magic numbers in many different places

What you can do is define another property in the IMessageHandler of Code and have an abstract base class that enforces that the Code must be set. By doing so you actually say "A MessageHandler is the one that knows which message it is responsible for".

public interface IMessageHandler
{
    IMessage Transform(IMessage message);
    int Code {get; set;}
}

public abstract class MessageHandlerBase : IMessageHandler
{
    public MessageHandlerBase(int code) 
    {
        Code = code;
    }

    public abstract IMessage Transform(IMessage message);
    public int Code {get; set;}
}

It will no longer receive a dictionary but an IEnumerable<IMessageHandler> - so to not have he numbers defined somewhere again. (you can implement it that it converts the IEnumerable to a dictionary so the search will still be in o(n) but that is implementation details.

For example message 2002(change user) is one message in the sending system. The receiving system doesn't have a change user but only a login (106) and logout(107).

Here you describe different services. Each service like this can still depend on the same type of factory described above but the initialization of that specific factory and of the Codes of each Handler in it can differ.

Again if I use Castle Windsor as the example - It has an option of configuring the dependencies via xml. So each service can have a section like this where in it's app.config you create the mapping of which behaviors you actually want to have and which Codes map to them

Answer 3

In general, this is an issue which would best be addressed by fixing it. If this is not an option, I'd use a switch-statement to handle the special behaviour. If you can, do as little as possible in the switch-cases and as much as possible in a general handler.

Answer 4

I'd set it on App.Config, And use Configuration Manager to get data.

Answer 5

Why not add an extension method to the Message class you are using that contains a property telling you the "type". If the "type" is well known in advance consider using an Enum to compare when making a decision. However you can assign the enum on construction of the message (for example) so you aren't doing the check for id each time you check on the type of message. If however the types is a long list, just use a string to denote type (more meaningful when read in code), again translate the code to the string in one place at at construction if possible.

Answer 6

I would have a set of SpecialMessageHandler classes and a StandardMessageHandler class then some kind of lookup (such has Dictionary). Instead of having if/switches you lookup the message id in the dictionary, if present use the specialised class, if not use the standard one. You could even specify the contents of the Dictionary outside of the code (e.g. web/app.config).

Answer 7

I'll write an overkill that probably will get many downvotes.

I would create an enum with the special codes (SpecialIds). Then, when checking if the number is a special one, I would transform the Id to a string.

With that string I would add the prefix "Special" which will look like Special_153

This name will be a class.

Then I would create a folder with all the special classes

Special_153.cs Special_154.cs

Every Special class would inherit from ISpecial which implements only a method Run.

The method Run would be called inside the constructor. The constructor would receive the full message object.

Then I would use it like this:

if (Enum.GetNames(typeof(SpecialIds)).Contains(message.id))
{
    //special behavior
    string id = message.Id.ToString();
    string prefix = "Special_";
    string className = prefix + id;

    Type t = Type.GetType(className);
    ConstructorInfo constructor = t.GetConstructor(new Type[] { message.GetType() });
    constructor.Invoke(message);
}

constructor.Invoke(message); will call the constructor with the argument message. Inside the class method Run you can modify all you want inside the object message.

This way you'll have one class per special Id, or you can call it handlers. They will be inside that folder and if you want to edit 1 of 200 handlers, you can easily reach the code.

Edit 1

Oh, and to finish, you could write an extension that would do that constructor call alone, and just call:

message.CheckSpecial();

Inside this extension you would check if the message is a special one. This way you could use this code in other places.