My approach to refactoring Paul Stack's monster switch

I read this blogpost a few days ago and was intrigued enough to have a go at refactoring its monster switch statement myself, during the Grand Prix. Here's my approach:

First task, get the creation of a "TaskClass" into a factory. A factory will need an interface to work with, so I defined the simplest one possible:

public interface ITaskClass
{
}

...and implemented it on each taskclass

public class Task1Class : ITaskClass
{
    public void ExtractTop10Data(DateTime dtLastRun)
    {
        return;
    }
}

Next, I created a TaskClassFactory, and duplicated the monster switch into the Create method

public static class TaskClassFactory
{
    public static ITaskClass CreateTaskClass(string name)
    {
        switch (name)
          {
            case "Task1":
                return new Task1Class();
            case "Task2":
                return new Task2Class();
            case "Task3":
                return new Task3Class();
            case "Task4":
                return new Task4Class();
        }
        return null;
    }
}

This gave me the opportunity to write some tests around the creation logic

[TestCase("Task1", typeof(Task1Class))]
[TestCase("Task2", typeof(Task2Class))]
[TestCase("Task3", typeof(Task3Class))]
[TestCase("Task4", typeof(Task4Class))]
public void CreateTest(string name, Type expectedType)
{
    Assert.IsInstanceOf(expectedType, TaskClassFactory.CreateTaskClass(name));
}

Then I introduced the factory into the monster switch (please ignore the smell, this is very much work in progress)

case "Task1":
try
{
    var task1Class = (Task1Class)TaskClassFactory.CreateTaskClass(task.TaskName);
    task1Class.ExtractTop10Data(dtLastRun);
    task.LastRunTime = DateTime.UtcNow;
    UpdateTaskRunStatus(TaskStatus.Good, task);
}
catch (Exception ex)
{
    UpdateTaskRunStatus(TaskStatus.Error, task);
    Logger.LogError("Error details here");
}
break;

Then I consolidated some of the repeated code; first the repeated catch block

try{
    // switch on taskname..
    //There were 22 task names in total making this method over 600 lines long!
    switch (task.TaskName)
    {
        case "Task1":
            var task1Class = (Task1Class)TaskClassFactory.CreateTaskClass(task.TaskName);
            task1Class.ExtractTop10Data(dtLastRun);
            task.LastRunTime = DateTime.UtcNow;
            UpdateTaskRunStatus(TaskStatus.Good, task);
            break;
        case "Task2":
            var task2Class = (Task2Class)TaskClassFactory.CreateTaskClass(task.TaskName);
            task2Class.CreateImageFeed();
            task.LastRunTime = DateTime.UtcNow;
            UpdateTaskRunStatus(TaskStatus.Good, task);
            break;
        case "Task3":
            var task3Class = (Task3Class)TaskClassFactory.CreateTaskClass(task.TaskName);
            task3Class.DestroyImageFeed();
            task.LastRunTime = DateTime.UtcNow;
            UpdateTaskRunStatus(TaskStatus.Good, task);
            break;
        case "Task4":
            var task4Class = (Task4Class)TaskClassFactory.CreateTaskClass(task.TaskName);
            task4Class.ProcessData();
            task.LastRunTime = DateTime.UtcNow;
            UpdateTaskRunStatus(TaskStatus.Good, task);
            break;
        default:
            break;
    }
}
catch (Exception)
{
    UpdateTaskRunStatus(TaskStatus.Error, task);
    Logger.LogError("Error details here");
}

and the common sweep-up tasks

try
{
    // switch on taskname..
    //There were 22 task names in total making this method over 600 lines long!
    switch (task.TaskName)
    {
        case "Task1":
            var task1Class = (Task1Class)TaskClassFactory.CreateTaskClass(task.TaskName);
            task1Class.ExtractTop10Data(dtLastRun);
            break;
        case "Task2":
            var task2Class = (Task2Class)TaskClassFactory.CreateTaskClass(task.TaskName);
            task2Class.CreateImageFeed();
            break;
        case "Task3":
            var task3Class = (Task3Class)TaskClassFactory.CreateTaskClass(task.TaskName);
            task3Class.DestroyImageFeed();
            break;
        case "Task4":
            var task4Class = (Task4Class)TaskClassFactory.CreateTaskClass(task.TaskName);
            task4Class.ProcessData();
            break;
        default:
            break;
    }
    task.LastRunTime = DateTime.UtcNow;
    UpdateTaskRunStatus(TaskStatus.Good, task);
}
catch (Exception)
{
    UpdateTaskRunStatus(TaskStatus.Error, task);
    Logger.LogError("Error details here");
}    

One of the tasks has a parameter to its processing method. I assumed that out of the 22 others not shown there would be more variation to the processing methods. With a nod of acknowledgement to YAGNI, I decided that each task class that needed a parameter to perform its function should be constructed with these parameters; this would mean that I could create a parameterless processing function in the interface. More on this later. Anyway, to enable different constructor parameters to be used in the factory, I created a parameter object and added it to the Create method

public struct Parameters
{
    public DateTime LastRunDate { get; set; }
}
...
public static ITaskClass CreateTaskClass(string name, Parameters createParams)

and in the monster switch:

TaskClassFactory.Parameters factoryParams = new TaskClassFactory.Parameters { LastRunDate = task.LastRunTime };
...
var task1Class = (Task1Class)TaskClassFactory.CreateTaskClass(task.TaskName, factoryParams);

This enabled me to define a parameterless DoWork method on the ITaskClass interface

public interface ITaskClass
{
    void DoWork();
}

This meant I needed to implement the DoWork method in each of the TaskClass implementations. For those with no parameters on the processing method, the implementation was a simple call to that method

public class Task2Class : ITaskClass
{
    public void CreateImageFeed()
    {
        return;
    }
    public void DoWork()
    {
        CreateImageFeed();
    }
}

The one with a parameter needed a little more scaffolding

public class Task1Class : ITaskClass
{
    private DateTime _lastRunDate;
    public Task1Class(DateTime lastRunDate)
    {
        _lastRunDate = lastRunDate;
    }
    public void ExtractTop10Data(DateTime dtLastRun)
    {
        return;
    }
    public void DoWork()
    {
        ExtractTop10Data(_lastRunDate);
    }
}

This forced the factory to change, to use Task1Class's new constructor

public static ITaskClass CreateTaskClass(string name, Parameters createParams)
{
    switch (name)
    {
        case "Task1":
            return new Task1Class(createParams.LastRunDate);

This meant I could hide the existing processing methods in the task classes

private void ExtractTop10Data(DateTime dtLastRun)
{
    return;
}

Next, I updated the monster switch to use the DoWork method for each implementation of ITaskClass

switch (task.TaskName)
{
    case "Task1":
        var task1Class = TaskClassFactory.CreateTaskClass(task.TaskName, factoryParams);
        task1Class.DoWork();
        break;
    case "Task2":
        var task2Class = TaskClassFactory.CreateTaskClass(task.TaskName, factoryParams);
        task2Class.DoWork();
        break;

Then, the money shot: I removed the monster switch itself

public void DoTask()
{
    MySqlConnection oSqlConn = new MySqlConnection();
    List<ScheduledTask> tasksToPerform = oSqlConn.GetTasks();
    try
    {
        foreach (ScheduledTask task in tasksToPerform)
        {
            TaskClassFactory.Parameters factoryParams = new TaskClassFactory.Parameters { LastRunDate = task.LastRunTime };
                
            try
            {
                var taskClass = TaskClassFactory.CreateTaskClass(task.TaskName, factoryParams);
                taskClass.DoWork();                       
                task.LastRunTime = DateTime.UtcNow;
                UpdateTaskRunStatus(TaskStatus.Good, task);
            }
            catch (Exception)
            {
                UpdateTaskRunStatus(TaskStatus.Error, task);
                Logger.LogError("Error details here");
            }
        }
    }
    catch (Exception ex)
    {
        OTLogger.LogGenericError(ex);
    }
    finally
    {
        SQLConnectionHelper.CloseConnection(oSqlConn);
    }
}

 The final code is available as a github:gist
Thanks to CraftyFella for the code formatting

For a Great UX, Consider the Colour Blind

Among my deficiencies is colour blindness; I'm not at the far end of the scale, but I have the traditional problems with reds/greens/browns. As Steve Fenton said (almost word-for-word) on his blog Test Your Website For Colour Blindness Issues, if you are not testing your website for colour-blindness, you are being stupid. I couldn't agree more.

So it's frustrating that when logging in to a major UK financial website, I am confronted by these buttons:

The vast majority of the thousands of you reading this will be thinking I don't get it; the rest of us will be thinking why did they choose the same colour for the "Log in" and "Clear" buttons?
The colours of these buttons are shown here, as various types of colour vision deficient people would see them (taken from the Color Blind Spectrum):

Normal	Protanopia	Deutanopia	Tritanoptia
#339900	#958600	#a58600	#518f9a
#cc0000	#786c1e	#886900	#cc1600

My colour blindness is Protanopia; you can see how I perceive the green and red buttons, and the lack of contrast between them.

With that in mind, imagine my delight when I pointed my web browser at the home page of the offending web site and saw a link that says High contrast

It does what it says on the tin; click it and you get this

In my naivity, I expected that when I clicked on the link to take me to the login screen, that too would be in High contrast mode. I expected the Log in and Clear buttons to be highly contrasted. This is what I got:

*sigh*  So nearly a great UX, but so far from one.

The Design of Everyday Things II or Don't Make Me Think

We have recently had a refit of the kitchen at work, and we have acquired some nice-looking canisters for tea, coffee and sugar. This is them:

How long does it take you to work out which one holds, for example, the sugar? Each time I make a cup of tea (and I drink a lot of tea), I need to put conscious effort into choosing the canister with tea in.

As Don Norman says in his book The Design Of Everyday Things, the designer probably won an award for them; and that's not a good thing. This is exactly what he talks about when he talks about knowledge "in the world" and not "in the head".

That's all, it's just a rant.