Its often assumed that legacy code should not be refactored because no one knows what worms are hidden in that can and what kind of monstrous bugs will be unleashed upon the users if that code is refactored. In some cases, it does seem to hold however, done in smaller chunks to the code that has fewer dependencies with well written unit tests, can actually slowly improve the overall health of the codebase.

Here is an example of a refactoring that I recently did on a ASP.NET/Silverlight project at work. The gist of this code is that its used to generate colour bands to be displayed on a thematic heatmap with score values assigned to each polygon on a geographical map (the details aren’t important here anyway). The old design (as shown in the below snippet) consisted of a class called BandGenerator that not only creates the bands but also is responsible for generating colours for those bands so if you needed to make any changes to the colour generation or add support for more colours, you will have to touch this class that should only be dealing with generating the bands.

public class BandGenerator
{
    public static ObservableCollection<Band> GenerateBands(double numberOfBands,      double rangeFrom,
double rangeTo,
bool catchHighScores,
string colorOption)
    {
        ObservableCollection<Band> bands = new ObservableCollection<Band>();
        try
        {
            double CurrentValue = rangeTo;
            double StartValue = rangeFrom;
            // ...details
            for (int i = 0; i <= numberOfBands - 1; i++)
            {
                double percentage = <algo for percentage calc>;

                bands.Add(new Band()
                {
                    FillColour = ColourByPercentage(percentage, colorOption).ToString(),
                });
            }
            //..details
        }
        catch (Exception ex)
        {
        }
        return bands;
    }

    public static Color ColourByPercentage(double Percentage, string colourOption)
    {
        byte colourvalue = (byte)Math.Floor(255 * Percentage);

        switch (colourOption)
        {
            case "Red":
                // calculate and return red
            case "Green":
                // calculate and return green
            case "Blue":
                // calculate and return blue
            case "Grey":
                // calculate and return grey
            case "Green-Red":
                // calculate and return green-red
            default:
                return ; //default colour
        }
     }
}

Clearly, its violating the Single Responsibility Principle and must be split into at least 2 classes: a colour generator and the band generator, with the band generator calling into the colour generator to source colours. Also the method to generate the bands can be simplified a little as well.

However, a colour generator in itself means nothing, there are different coloured bands that it should provide so its perhaps more appropriate to make it an abstract ColourBand base class like so:

public abstract class ColourBand
{
    public abstract string BandColourName { get; }
    public abstract Color GetColour(double colourIntensityPercentage);
}

Because the UI needs to display the names of the colour bands available for the user (may be bound to a drop down control or a list control) the abstract class needs to have an abstract BandColourName property that will be implemented by child classes for each of the colours that we are going to support. The GetColour method will be responsible for generating the RGB values based on a passed in colour intensity with each band becoming more and more intense as it goes down. So the algorithm to manipulate the RGB values will vary based on the colour desired therefore the child classes will need to provide their own implementations for this method. Here’s the implementation for YellowToPurpleBand child class:

public class YellowToPurpleBand : ColourBand
{
    public override string BandColourName
    {
        get
        {
            return "Yellow-Purple";
        }
    }

    public override Color GetColour(double colourIntensityPercentage)
    {
        // implementation is beyond the scope for this post.
    }
}

Next, the new ColourBandGenerator class can be refactored like so:

public class ColourBandGenerator
{
    private int numberOfBands;
    private int startValue;
    private int endValue;
    private ColourBand colourBand;
    private bool shouldCatchHighScores;

    // inject all the values and the ColourBand class instance which at runtime
    // will be pointing to the specific colour band selected from the UI. This is also
    // also the fundamental of Dependency Injection.
    public ColourBandGenerator(int numberOfBands, int startValue, int endValue, bool shouldCatchHighScores, ColourBand colourBand)
    {
        this.colourBand = colourBand;
        this.endValue = endValue;
        this.startValue = startValue;
        this.numberOfBands = numberOfBands;
        this.shouldCatchHighScores = shouldCatchHighScores;
    }

    public ObservableCollection<Band> GenerateBands()
    {
        for (int i = 0; i < this.numberOfBands; i++)
        {
            // band generation details beyond the scope of this post and have been removed
            double colourShiftPercentageValue = <algorithm to compute colour intensity  required>
            bands.Add(new Band()
            {
                // details
                FillColour = this.colourBand.GetColour(colourShiftPercentageValue).ToString(),
            });
        }

        return bands;
    }
}

As you can see, now the ColourBandGenerator class only depends on the abstract ColourBand class which means we can easily add more colour bands without ever needing to change anything in the ColourBandGenerator itself. The instance of the appropriate ColourBand class is passed in from the UI through the user selection which then gets injected into the ColourBandGenerator class honoring the Dependency Inversion principle.

Another problem (at least in this case) is I don’t want to manually bind the UI to the available colour bands, there should be a mechanism that can load all the implementations of the ColourBand class and bind the UI dynamically such that anytime a new colour band class gets added it automatically appears in the UI without any changes required to add it to the collection of available colour bands. Turns out, a quick “dependency loader” helper can do the trick:

public class AvailableColourBandCollection
{
    public static ObservableCollection<ColourBand> GetAll()
    {
        ObservableCollection<ColourBand> bandColourTypes = new     ObservableCollection<ColourBand>();
        Type[] allTypesInThisAssembly = Assembly.GetExecutingAssembly().GetTypes().Where(x => x.IsSubclassOf(typeof(ColourBand))).ToArray();

        foreach (Type t in allTypesInThisAssembly)
        {
            var instance = Activator.CreateInstance(t);
            bandColourTypes.Add(instance as ColourBand);
        }

        return bandColourTypes;
    }
}

It uses reflection to load all the types that inherit from the abstract ColourBand class, creates an instance of each, adds to the collection and returns to the client code that then uses Silverlight XAML binding (in this case) to bind a dropdown control to this collection of available colour bands. And that’s it!

colour-bands
Silverlight UI for Colour Bands

This code is now far loosely coupled than the previous monolithic structure, its more maintainable as each class only has one reason to change and its easily extensible with little to no impact on other parts of the system. In other words its more SOLID!