Have you ever had to write a long series of if/then statements or a giant switch statement that extends for a page or two of your code? Want an easy way to increase the readability of your code, increase it's maintainability index, and decrease it's cyclomatic complexity? Let's see an example problem and then a possible solution.
Let's start off by considering the following array of anonymous types:
var items = new[]
{
new {Operation = Operation.Add, Value1 = 1, Value2 = 2},
new {Operation = Operation.Subtract, Value1 = 4, Value2 = 3},
new {Operation = Operation.Multiply, Value1 = 5, Value2 = 6},
new {Operation = Operation.Divide, Value1 = 8, Value2 = 4},
new {Operation = Operation.Add, Value1 = 9, Value2 = 10},
new {Operation = Operation.Subtract, Value1 = 12, Value2 = 11},
new {Operation = Operation.Multiply, Value1 = 13, Value2 = 14},
new {Operation = Operation.Divide, Value1 = 16, Value2 = 4},
new {Operation = Operation.Add, Value1 = 17, Value2 = 18},
new {Operation = Operation.Subtract, Value1 = 6, Value2 = 3},
new {Operation = Operation.Multiply, Value1 = 8, Value2 = 8},
new {Operation = Operation.Divide, Value1 = 10, Value2 = 2}
};
I need to iterate over this array and execute the given Operation on Value1 and Value2 properties to produce a list of results A brute force method of doing this is as follows:
var results = new List<int>();
foreach (var item in items)
{
switch (item.Operation)
{
case Operation.Add:
results.Add(item.Value1 + item.Value2);
break;
case Operation.Subtract:
results.Add(item.Value1 - item.Value2);
break;
case Operation.Multiply:
results.Add(item.Value1*item.Value2);
break;
case Operation.Divide:
results.Add(item.Value1/item.Value2);
break;
default:
throw new ArgumentOutOfRangeException();
}
}
The switch statement is pretty straightforward and simple to implement in this example. Visual Studio 2013 Code Analysis is giving the code a maintainability index of 54 and a cyclomatic complexity of 6. What if I needed more lines of code to handle each operation? This could turn into a mess to test, debug, and maintain.
A possible solution to this problem is to replace your switch with a dictionary. I realize that seems odd to replace a control statement with a data structure, but refactoring to this approach will simplify your code. In our example, each item in the array has an Operation to be performed on the Value1 and Value2 properties. I can setup my dictionary with the key value being the Operation enumeration, and the value being the Action, Func, or method group that performs the operation.
var operationMethods = new Dictionary<Operation, Func<int, int, int>>
{
{Operation.Add, Add},
{Operation.Subtract, Subtract},
{Operation.Multiply, Multiply},
{Operation.Divide, Divide}
};
In the example above, I'm using the following methods:
public int Add(int left, int right)
{
return left + right;
}
public int Subtract(int left, int right)
{
return left - right;
}
public int Multiply(int left, int right)
{
return left*right;
}
public int Divide(int left, int right)
{
return left/right;
}
With the dictionary approach, our code to iterate over the items and execute the individual operations looks like this:
var results = new List<int>();
foreach (var item in items)
{
results.Add(operationMethods[item.Operation].Invoke(item.Value1, item.Value2));
}
VS2013 gives the refactored code a maintainability index of 63 and a cyclomatic complexity of 2. Our code is still doing the same thing as the switch was, but we have broken the code down to simpler pieces that are more readable and more easily to wrap unit tests around.
I find this to be a good approach to take when the "cases" of our switch statement are known up front (such as using an enumeration). If it is possible that you will not have all the cases or keys in the dictionary, you can still use this approach but you will want to handle things more gracefully with the Dictionary's TryGetValue method like so:
Func<int, int, int> operation;
if (operationMethods.TryGetValue(item.Operation, out operation))
{
results.Add(operation(item.Value1, item.Value2));
}
else
{
// TODO: Handle case of the missing case/key
}
To tie it all together, here is a Gist with the code I discussed in this post.
I hope this helps. If you have any questions or comments, please use the comments form below. Thanks.
