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;
.csharpcode-wrapper, .csharpcode-wrapper pre {
background-color: #f4f4f4;
border: solid 1px gray;
cursor: text;
font-family: consolas, ‘Courier New’, courier, monospace;
font-size: 8pt;
line-height: 12pt;
margin: 20px 0px 10px 0px;
max-height: 200px;
overflow: auto;
padding: 4px 4px 4px 4px;
width: 97.5%;
}
.csharpcode-wrapper pre {
border-style: none;
margin: 0px 0px 0px 0px;
overflow: visible;
padding: 0px 0px 0px 0px;
}
.csharpcode, .csharpcode pre, .csharpcode .alt {
background-color: #f4f4f4;
border-style: none;
color: black;
font-family: consolas, ‘Courier New’, courier, monospace;
font-size: 8pt;
line-height: 12pt;
overflow: visible;
padding: 0px 0px 0px 0px;
width: 100%;
}
.csharpcode pre {
margin: 0em;
}
.csharpcode .alt {
background-color: white;
}
.csharpcode .asp {
background-color: #ffff00;
}
.csharpcode .attr {
color: #ff0000;
}
.csharpcode .html {
color: #800000;
}
.csharpcode .kwrd {
color: #0000ff;
}
.csharpcode .lnum {
color: #606060;
}
.csharpcode .op {
color: #0000c0;
}
.csharpcode .preproc {
color: #cc6633;
}
.csharpcode .rem {
color: #008000;
}
.csharpcode .str {
color: #006080;
}
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;
.csharpcode-wrapper, .csharpcode-wrapper pre {
background-color: #f4f4f4;
border: solid 1px gray;
cursor: text;
font-family: consolas, ‘Courier New’, courier, monospace;
font-size: 8pt;
line-height: 12pt;
margin: 20px 0px 10px 0px;
max-height: 200px;
overflow: auto;
padding: 4px 4px 4px 4px;
width: 97.5%;
}
.csharpcode-wrapper pre {
border-style: none;
margin: 0px 0px 0px 0px;
overflow: visible;
padding: 0px 0px 0px 0px;
}
.csharpcode, .csharpcode pre, .csharpcode .alt {
background-color: #f4f4f4;
border-style: none;
color: black;
font-family: consolas, ‘Courier New’, courier, monospace;
font-size: 8pt;
line-height: 12pt;
overflow: visible;
padding: 0px 0px 0px 0px;
width: 100%;
}
.csharpcode pre {
margin: 0em;
}
.csharpcode .alt {
background-color: white;
}
.csharpcode .asp {
background-color: #ffff00;
}
.csharpcode .attr {
color: #ff0000;
}
.csharpcode .html {
color: #800000;
}
.csharpcode .kwrd {
color: #0000ff;
}
.csharpcode .lnum {
color: #606060;
}
.csharpcode .op {
color: #0000c0;
}
.csharpcode .preproc {
color: #cc6633;
}
.csharpcode .rem {
color: #008000;
}
.csharpcode .str {
color: #006080;
}
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: }
.csharpcode-wrapper, .csharpcode-wrapper pre {
background-color: #f4f4f4;
border: solid 1px gray;
cursor: text;
font-family: consolas, ‘Courier New’, courier, monospace;
font-size: 8pt;
line-height: 12pt;
margin: 20px 0px 10px 0px;
max-height: 200px;
overflow: auto;
padding: 4px 4px 4px 4px;
width: 97.5%;
}
.csharpcode-wrapper pre {
border-style: none;
margin: 0px 0px 0px 0px;
overflow: visible;
padding: 0px 0px 0px 0px;
}
.csharpcode, .csharpcode pre, .csharpcode .alt {
background-color: #f4f4f4;
border-style: none;
color: black;
font-family: consolas, ‘Courier New’, courier, monospace;
font-size: 8pt;
line-height: 12pt;
overflow: visible;
padding: 0px 0px 0px 0px;
width: 100%;
}
.csharpcode pre {
margin: 0em;
}
.csharpcode .alt {
background-color: white;
}
.csharpcode .asp {
background-color: #ffff00;
}
.csharpcode .attr {
color: #ff0000;
}
.csharpcode .html {
color: #800000;
}
.csharpcode .kwrd {
color: #0000ff;
}
.csharpcode .lnum {
color: #606060;
}
.csharpcode .op {
color: #0000c0;
}
.csharpcode .preproc {
color: #cc6633;
}
.csharpcode .rem {
color: #008000;
}
.csharpcode .str {
color: #006080;
}
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;
.csharpcode-wrapper, .csharpcode-wrapper pre {
background-color: #f4f4f4;
border: solid 1px gray;
cursor: text;
font-family: consolas, ‘Courier New’, courier, monospace;
font-size: 8pt;
line-height: 12pt;
margin: 20px 0px 10px 0px;
max-height: 200px;
overflow: auto;
padding: 4px 4px 4px 4px;
width: 97.5%;
}
.csharpcode-wrapper pre {
border-style: none;
margin: 0px 0px 0px 0px;
overflow: visible;
padding: 0px 0px 0px 0px;
}
.csharpcode, .csharpcode pre, .csharpcode .alt {
background-color: #f4f4f4;
border-style: none;
color: black;
font-family: consolas, ‘Courier New’, courier, monospace;
font-size: 8pt;
line-height: 12pt;
overflow: visible;
padding: 0px 0px 0px 0px;
width: 100%;
}
.csharpcode pre {
margin: 0em;
}
.csharpcode .alt {
background-color: white;
}
.csharpcode .asp {
background-color: #ffff00;
}
.csharpcode .attr {
color: #ff0000;
}
.csharpcode .html {
color: #800000;
}
.csharpcode .kwrd {
color: #0000ff;
}
.csharpcode .lnum {
color: #606060;
}
.csharpcode .op {
color: #0000c0;
}
.csharpcode .preproc {
color: #cc6633;
}
.csharpcode .rem {
color: #008000;
}
.csharpcode .str {
color: #006080;
}
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.