How many times have you had to work with some Legacy code? Was it readable? Silly question… But what was the main problem? If the code was written in an unclear way and you did not have any idea how to refactor it please visit my post under this LINK. I have recently stumbled across another problem: magic strings. And boy, I was surprised by human ingenuity once again! Imagine having a class whose source code you cannot change due to technical or business reasons. It looks like this:

class Worlflow
{
    public int Type; 
    public string CurrentStep;

    // workflow data
}

At first glance it seems to be ok, but when you find a method dependent on this class that looks like this:

bool CanUserAbort(Worlflow workflow)
{
    if (workflow.CurrentStep != "Cld")
    {
        if (workflow.Type == 0 && workflow.CurrentStep == "St")
        {
            return false;
        }
        if (workflow.Type == 0 && workflow.CurrentStep == "Pd")
        {
            return false;
        }
        if (workflow.Type == 0)
        {
            return true;
        }
        if (workflow.Type == 1 && workflow.CurrentStep == "Acc")
        {
            return false;
        }
        if (workflow.Type == 1)
        {
            return true;
        }
    }
    return false;
}

you may quickly change your mind. Especially if you have to modify this method due to Business Rule change or any other reason. Just to be sure that everyone is on the same page, the main problems are:

• CurrentStep is compared with some strange looking strings,
• Type is at best unclear, not to say it does not give a clue about its information,
• method structure suggests that it has been incrementally changed and other programmers were mainly adding new ‘if’ statements rather than refactoring the existing code,

Enum but not Enum

Obviously both workflow.Type and workflow.CurrentStep should be replaced by enums (instead of string and integer). But since we cannot change Workflow class this solution is unavailable. One of the most common solutions is to wrap one Workflow class in another that would act as a layer translating information between Workflow class and the rest of the application. But this is not the pattern I would like to share with you today. Instead I will create classes that would act like enums and will translate data from string and int to more reliable and readable form. How to do this? Here is a list of things that must be done:

• Since we do not create an instance of enum every time we use it there would be no point creating another instance of whatever we are going to use – so there will be some statics involved as well as a private constructor.
• As we need to pass data and be able to customize our “enums” we will need to pass a parameter in the constructor.
• Although workflow.CurrentStep and workflow.Type will be a different type from what we are about to create, we can always override ‘==’ and ‘!=’ operators.

The outcome of this assumptions may look like this:

class Step
{
    public static readonly Step Accepted = new Step("Acc");
    public static readonly Step Send = new Step("St");
    public static readonly Step Paid = new Step("Pd");
    public static readonly Step Closed = new Step("Cld");

    private string _step;
    private Step(string step)
    {
        _step = step;
    }

    public bool Equals(string name)
    {
        return _step.Equals(name);
    }

    public static bool operator ==(string name, Step obj)
    {
        return obj.Equals(name);
    }

    public static bool operator !=(string name, Step obj)
    {
        return !(obj.Equals(name));
    }
}

class WorkflowType
{
    public static readonly WorkflowType Purchase = new WorkflowType(0);
    public static readonly WorkflowType Refound = new WorkflowType(1);

    private int _type;
    private WorkflowType(int type)
    {
        _type = type;
    }

    public bool Equals(int type)
    {
        return _type.Equals(type);
    }

    public static bool operator ==(int type, WorkflowType obj)
    {
        return obj.Equals(type);
    }

    public static bool operator !=(int type, WorkflowType obj)
    {
        return !(obj.Equals(type));
    }
}

And these new classes can be used in CanUserAbort method like this:

bool CanUserAbort(Worlflow workflow)
{
    if (workflow.CurrentStep != Step.Closed)
    {
        if (workflow.Type == WorkflowType.Purchase && workflow.CurrentStep == Step.Send)
        {
            return false;
        }
        if (workflow.Type == WorkflowType.Purchase && workflow.CurrentStep == Step.Paid)
        {
            return false;
        }
        if (workflow.Type == WorkflowType.Purchase)
        {
            return true;
        }
        if (workflow.Type == WorkflowType.Refound && workflow.CurrentStep == Step.Accepted)
        {
            return false;
        }
        if (workflow.Type == WorkflowType.Refound)
        {
            return true;
        }
    }
    return false;
}

As you can see, all strings and integers were replaced by our custom ‘enum’ classes. On could also change multiple ‘if’ statements to switch/case structure.  But let’s not stop here. After a short refactor we get:

bool CanUserAbort(Worlflow workflow)
{
    if (workflow.CurrentStep == Step.Closed) 
    { 
        return false; 
    }
    if (workflow.Type == WorkflowType.Purchase)
    {
        return !(workflow.CurrentStep == Step.Send || workflow.CurrentStep == Step.Paid);
    }
    if (workflow.Type == WorkflowType.Refound)
    {
        return workflow.CurrentStep != Step.Accepted;
    }
    return false;
}

Which looks way better than the original method and is incomparably more readable. This approach gives us significant benefits:

• Workflow class has not been changed so there is no way we can cause errors in other parts of the application.
• Since all strings and integers were removed, there is no real danger of introducing a bug to the application by misspelling a single item.
• All comparisons are now made in an easily readable form that is closer to Business Rules in the application rather than physical data implementation.
• Equals method is way safer option to compare two strings than ‘==’ operator.

Of course one can argue that this solution brings some additional calculation overhead. But to be fair this overhead is negligible in the scope of the entire application. And an application that uses magic strings is probably also not very optimized.

Additionally we can  extend  ‘Enum’ classes by inheriting from  IEquatable<T> and IComparable Interfaces that will allow to use CompareTo() and Equals() methods throughout the system – but that is a subject for another article.

There is also another solution. We can use static class with constant string fields like this:

public static Step
{
    public const string Accepted = "Acc";
    public const string Send = "St";
    public const string Paid = "Pd";
    public const string Closed = "Cld";
}

This will also allow the refactored CanUserAbort method to look exactly like in previous example. The drawback is that you will not be able to do something like this:

public void ProceedStep(Step step){…}

And instead it would still look like this:

public void ProceedStep(string step){…}

Which brings us back to original problem with magic strings in random places in our code.

What about real Enums?

Well we can always use real C# enums. Consider the following Enums:

public enum WorkflowType 
{
    Purchase = 0,
    Refound = 1
}

public enum Step
{
    Acc = 0,
    St,
    Pd,
    Cld
}

And we can use them in CanUserAbort method like this:

bool CanUserAbort(Worlflow workflow)
{
    if (workflow.CurrentStep == Step.Cld.ToString())
    {
        return false;
    }
    if (workflow.Type == (int)WorkflowType.Purchase)
    {
        return !(workflow.CurrentStep == Step.St.ToString() || workflow.CurrentStep == Step.Pd.ToString());
    }
    if (workflow.Type == (int)WorkflowType.Refound)
    {
        return workflow.CurrentStep != Step.Acc.ToString();
    }
    return false;
}

It is usually a good idea to set one enum value as default (Acc = 0) because adding next value on top of all enum values may mess up the application if default value is not explicitly defined.  Although this solution will work as well as all previous ones I personally (and subjectively) object to it. There are several reasons:

• To compare two string values we must use ToString method every time we do the comparison.
• Step enum has strange values like Step.Cld, Step.St, Step.Pd or Step.Acc which are no more descriptive than magic strings from the original method.
• In order to be compared with integer WorkflowType enum must be casted to integer each time the comparison takes place.
• In this particular scenario enums do not bring much help and the code is still pretty unclear.

Conclusion

Usually there are many different ways of programming in C#, but just because some options exist does not mean that they should be used. Always consider not only short term consequences (e.g. development speed) of creating code but also long term ones (e.g. maintainability). From my own experience most (if not all) things that we neglect or leave behind with ‘To Do: refactor ’ comment will eventually come back and bite you in the ass. So deal with them at once. Now go and clean up your code until it is not too late and it becomes Legacy code. And please let me know how it went.

Tags: .NET, Best practices, C#, Software