My approach to refactoring a monster switch statement

Reply

Kristof Claes
4/16/2012 8:21:45 AM #

I think I would have handled this in the same way. Maybe I wouldn't expose the Dictionary of your TaskRunner directly but provide something like a TryRun(string task) method instead.

Reply

Nathan Gloyn
4/16/2012 9:52:37 AM #

I would have done similar but would have extracted the common parts out e.g. LastRunTime & UpdateTaskRunStatus to a method and then looked to inject an Func into that method that did the specific functionality required and returned the task from the Func to allow the common code to run.

Just a note you are still violating the Open/Closed prinicple as if you need to add another class then you are having to modify the TaskRunner class.

Reply

Mark Rendle
4/16/2012 11:00:23 AM #

I know I'm always on about functional programming, but I think in this instance a more object-oriented approach is valid. This is essentially the Command pattern, so having separate classes to process each type of task seems appropriate, and also allows the exception-handling and task-status-updating to be handled in a base class.

My version here: https://gist.github.com/2397400

Reply

Stuart
4/16/2012 11:17:03 AM #

I've refactored this pattern lots before.

My normal way of doing it is using reflection.

I set up an attribute like [Func("KeyName")] on the methods and then call the methods.

Bizarrely I can't think of a C# version to point you to... but here's Java: I tried to persuade some Java guys I was working with to do the same thing - their case was worse as Java doesn't seem to like switching on strings. They had a "switch" method like github.com/.../Switcheroo.java

After refactoring (including splitting across files) it looked more like:
-github.com/.../LayerMethods.java
and
- github.com/.../MoreLayerMethods.java

Oh... and the Java guys said no - and their switch file currently has a single method with maybe 50 switches and 900 lines of code it it...

Reply

Stuart
4/16/2012 11:19:15 AM #

oops - sorry for the typos.

also just to warn you that the example I pointed to was the even more generic case - where the switch function takes and object[] array as the argument and where each switch block expects different parameters - so there's lots of casting of parameters done in the switch.

Reply

Howard
4/16/2012 11:23:04 AM #

There's a lot of your code in the examples above that's basically the same in each case.

The execute is almost always the same set of code with only one or two lines different.

I would probably have refactored to create a base class to contain that code, and then subclass each to perform the unique actions. That seems to me to fit the OOP pattern better:

Reply

Howard
4/16/2012 11:57:33 AM #

Hmm where did my first comment go??

OK here is my suggestion. There's a lot of common code in your methods. The suggestion I'd have is map the task name to the class name and instantiate that way...

My version: https://gist.github.com/2397758

Reply

Chris Vann
4/16/2012 7:57:37 PM #

Whenever I find myself in this scenario, I create a parent ICustomTask interface (ICustomTask can be renamed to indicate some specific area of functionality). This ICustomTask interface has a property, ideally an enum, but it can be a string also, that defines which task each implementation will handle. It also defines a Run method which handles the actual task work. Then, either through automatic registration via reflection, or manual registration via a helper class, these classes are registered so that the task runner class can find them. From there, it's as simple as doing foreach (var impl in myTaskRunners) { if (impl.Task == "MyTaskName") task.Run(); }

Reply

Michael
4/16/2012 8:42:45 PM #

I like Howard's approach.  I was thinking of a way to do it similarly with an interface that ScheduledTask inherits from (which I assume all concrete tasks inherit from ScheduledTask) that exposes a method for processing.  

So you would end up having something similar to:
foreach (ScheduledTask task in tasksToPerform)
            {
                DateTime dtLastRun = task.LastRunTime;

                task.DoWork();
            }

Reply

richard s
4/18/2012 3:21:14 PM #

The main problem I see here is having your separate task methods in the same class. Hence adding a new task would involve adding a new method to this class (eg private void ProcessTask5(ScheduledTask task) ).

If you made your dictionary values reference a Command pattern type interface rather than delegate Action<> methods and implement Command->Execute for each task then all you would have to do is add a new implementation of Command to your dictionary in the initialisation stage.

This isn't so much a problem because you could make this initialisation dynamic from , say a config file and use Assembly.CreateInstance() from the read-in class name, or just hard code it. Whichever way it is a smaller change.

Also - It may be an idea not to check if the dictionary contains the task name before you attempt to use it, else you won't notice if you have missed a task out, or misspelt it, in your dictionary.

Reply

Mark Jones
5/13/2012 6:21:16 PM #

I came late to the party, sorry.

My solution consists of:
- abstracting the performing of a task to an interface
- moving the responsibility of creating the taskClass to a factory

My solution is here https://gist.github.com/2689356

Bear in mind that I have the benefit of hindsight, no responsibility to prove my changes via unit tests and only 4 of the original 22 TaskClass objects...

Would like to know what others think of this approach.