Real-World Refactoring: Available Cars
Are all of those if..then statements really necessary? In today's post, we look at refactoring a long list of if..then statements into an easy two-liner.
Refactoring code is something developers should focus on and, as my one friend said, "Relentlessly Refactor."
Refactoring makes code easier to read and easier to maintain so it makes sense to practice (i.e. katas or competitions) every once in a while to keep your skills sharpened.
About a week ago, I was working with a client and came across some code that didn't sit right with me so I decided to ask my newsletter readers if they could refactor the code.
In Friday's newsletter, I added the exercise to the newsletter.
The exercise contained a simple scenario.
There was one method to create a list of strings based on a number of conditions. If a condition was met, the related string was included in the list and returned from the calling method.
The code in question was this:
private string[] GetAvailable(CarRequest request)
{
var result = new List<string>();
if (request.State == "CA")
{
result.Add("BMW");
}
if (request.State == "CA" && request.Imported)
{
result.Add("Ferrari");
}
if (request.State == "MA")
{
result.Add("Jeep Cherokee");
}
if (request.State == "PA")
{
result.Add("Jeep Cherokee");
}
return result.ToArray();
}
Based on the CarRequest, we examine the buyer's state and determine if they want an import. Once we examine the buyer's request, we return a list of cars available for them.
As you can see, this can get a little out of hand. Of course, you could make this data-driven with a set of rules, but for now, let's just look at an initial refactoring for this exercise.
Why Refactor This?
While I've mentioned this before, there are two immediate reasons that come to mind when I see this type of code.
Eliminate Cyclomatic Complexity
One of my friends early in my career said,
"If you were to create a truly object-oriented application, it would have no if..then statements in the code."
I think of his comment every single time I see code like this.
Think about how you refactor this code to not use if..then statements.
The more if..then statements you have in your code, the more the cyclomatic complexity will increase. In other words, "spaghetti code."
Every condition used can send you down a different path making it harder to follow. The less if..then statements in the code, the easier it is to understand and maintain.
Open-Closed Principle
If you're familiar with the SOLID principles, you already know where I'm going with the Open-Closed Principle.
The Open-Closed principle means your code should be "Open for extension, Closed for modification." This gives developers a means to extend the code without copy/pasting.
Your code should be easy enough to extend the code without much effort.
A [Possible] Solution
So let's get back to our exercise.
The pattern I see is a condition (request.State ==
"CA"
) and a result ("BMW"). There can be other pairing patterns like a condition/Action pairing. If a condition is met, kick-off this Action.
As a first pass, I see this code refactored into this:
private string[] GetAvailable(CarRequest request)
{
var conditions = new Dictionary<string, bool>
{
{ "BMW", request.State == "CA" },
{ "Ferrari", request.State == "CA" && request.Imported },
{ "Jeep Cherokee", request.State == "MA"},
{ "Jeep Cherokee", request.State == "PA"}
};
return conditions
.Where(e=> e.Value)
.Select(e=> e.Key)
.ToArray();
}
These two lines of code provide all of the functionality of the previous code listed above, simplifies the code, and returns a list of cars the buyer can purchase based on where they're willing to travel.
How much complexity did this introduce compared to an if..statement or inline-if?
Conclusion
This simple exercise made me look back over older code and think about how to refactor complex code into newer and simpler approaches. With this "first pass" of refactoring, you can even apply this concept to switch statements.
If you are looking for more ways to refactor code, check out the Real-World Refactoring Collection.
As readers of C#, was this an easier approach or did it seem harder to understand? How did you refactor this out? Post your comments below and let's discuss.