Tim Rayburn
Tim Rayburn
Lead with grace and be wholly you.

Break the Habit : Nested If Statements

It is time to break some bad habits, and today we take some code written by a man I like (but who will remain nameless and linkless) that is in desperate need of refactoring. But this problem is not a problem exclusive to this very bright developer, it is a bad habit that we as an industry have picked up somewhere along the line and I'm here to say "Stop The Insanity!".

Consider the following code:

 1: if (_privateDataObject.Methodology != null)
 2: { 
 3: if (_privateDataObject.Methodology == "00") 
 4: { 
 5: if (_privateDataObject.Amount != null)
 6: { 
 7: if (IsNumeric(_privateDataObject.Amount)) 
 8: { 
 9: if (Convert.ToDecimal(_privateDataObject.Amount )==0 ) 
10: { 
11: bReturnValue =true; 
12: } 
13: else 
14: { 
15: 
16: bReturnValue =false; 
17: } 
18: } 
19: else 
20: { 
21: 
22: bReturnValue =false; 
23: } 
24: } 
25: else 
26: { 
27: 
28: bReturnValue =false; 
29: } 
30: } 
31: else 
32: { 
33: 
34: bReturnValue =false; 
35: } 
36: } 
37:  
38: return bReturnValue;

 

This decision weighs in at over 38 lines. Let's look at what the warning signs in this method should have been that it needed to be coded differently.

The first and most important is that this follows a very basic pattern we see in code, we are seeking for single complex condition and want to return a "true" if we find it, and "false" in all other cases. This pattern is so basic it is taught in every Intro to Programming course ever. The pattern has a common name "Boolean AND" and is a part of every language (that I know of) on the planet. Refactored to accomidate for this pattern we end up with :

  

 1: if (_privateDataObject.Methodology != null &&
 2: _privateDataObject.Methodology == "00" && 
 3: _privateDataObject.Amount != null &&
 4: IsNumeric(_privateDataObject.Amount) && 
 5: Convert.ToDecimal(_privateDataObject.Amount ) == 0) 
 6: {
 7: bReturnValue =true; 
 8: } 
 9: else 
10: { 
11: bReturnValue =false; 
12: } 
13:  
14: return bReturnValue;

From 38 lines down to 14, not bad. And this version is not even as short (in line count) as it could be in favor of readability (spreading the expression across multiple lines) and to include braces (which are optional with only 1 command).

Now that we've tamed the wild nested if, let's look at another problem. Why set a value, only to immediately return that value once you've decided on the result? Just return the value immediately. This small win nets us the following code: 

 1: if (_privateDataObject.Methodology != null &&
 2: _privateDataObject.Methodology == "00" && 
 3: _privateDataObject.Amount != null &&
 4: IsNumeric(_privateDataObject.Amount) && 
 5: Convert.ToDecimal(_privateDataObject.Amount ) == 0) 
 6: {
 7: return true; 
 8: } 
 9: else 
10: { 
11: return false; 
12: }

From 14 to 12, not bad, and even less un-needed code. The final refactoring is based on the fact that this code is searching for a boolean value using boolean expressions. That means that to reach true "ruthless" refactoring we should end up with the following:

 
 1: return _privateDataObject.Methodology != null &&
 2:_privateDataObject.Methodology == "00" && 
 3:_privateDataObject.Amount != null &&
 4:IsNumeric(_privateDataObject.Amount) && 
 5:Convert.ToDecimal(_privateDataObject.Amount ) == 0;

Final result, 5 lines of code, same functionality. Important note to the Visual Basic developers out there, && is the equivalent of "AndAlso" in VB, not simply "And", hence why this code does not blow up with a NullReferenceException.

I implore every developer reading this to get into the habit of considering how to refactor code as soon as you've got it working, and I mean on a method by method basis. Even if you're not using a "TDD" methodology, this is a very good habit to get into because every time we change code we've introduced the chance to improve either the readability or performance of the code as well. It is natural to reach "working" code and want to move on, but spending 10 minutes to examine how you got it working will pay off in the long run, I assure you.