As developers the readability of our code is what determines
how easily either we ourselves or other developers can maintain our code.
Sometimes we can accomplish the same functionality in fewer lines of code,
or using some fancy tricks, but this can be bad for maintainability.
Sure, there are some situations where some of the code is repeated unnecessarily,
or may not be needed at all, and can be made both simpler and easier to read by
refactoring it. But rewriting readable
code in fewer lines such that anyone who looks at it will spend considerable time
understanding what it is doing is no good.
Extensive use of the ternary conditional operator is
one sign your code might not be as readable as it should be.
For example, the following code works correctly and in my opinion is pretty
readable as to what its intended functionality is supposed to be:
if
(!productStartAdded)
{
// Add with an OR unless worklist criteria count is 0
if
(filterCount == 0)
{
worklistCriteria.AddFilterField(WCLogical.And, WCField.ProcessStatus,
WCCompare.Equal, k2ProcessInstanceStatuses[j]);
filterStringBuilder.Append(" && ProcessStatus == " + k2ProcessInstanceStatuses[j].ToString());
}
else
{
worklistCriteria.AddFilterField(WCLogical.Or, WCField.ProcessStatus,
WCCompare.Equal, k2ProcessInstanceStatuses[j]);
filterStringBuilder.Append(" || ProcessStatus == " + k2ProcessInstanceStatuses[j].ToString());
}
productStartAdded
= true;
}
else
{
worklistCriteria.AddFilterField(WCLogical.And, WCField.ProcessStatus,
WCCompare.Equal, k2ProcessInstanceStatuses[j]);
filterStringBuilder.Append(" && ProcessStatus == " + k2ProcessInstanceStatuses[j].ToString());
}
Note that the example code above is using the K2ROM
API for K2.NET. If there were an overload
of the AddFilterField method that took just the And or Or, the code would be simplifyable
and retain its readability but unfortunately there is no such overload.
But say we decide that we can do some fancy maneuvering
and rewrite these 19 lines of code in just three lines!
Wow, that would be cool, right?
Take a look at those three lines, which have the same functionality as the original
19:
worklistCriteria.AddFilterField(productStartAdded
&& filterCount != 0 ? WCLogical.And
: WCLogical.Or, WCField.ProcessStatus,
WCCompare.Equal, k2ProcessInstanceStatuses[j]);
filterStringBuilder.Append((productStartAdded
&& filterCount != 0 ? " && "
: " || ") + " ProcessStatus
== " + k2ProcessInstanceStatuses[j].ToString());
productStartAdded
= true;
That is downright amazing.
The compiler is going to thank us for that, right?
Or is it? Well, the original version
results in 91 lines of IL code, while the rewritten version is just 49 lines of
IL. That is certainly not the roughly
6:1 ratio that we saw in the C# code, nor is it even 2:1 in IL.
One might expect about a 46% performance boost based
on those IL line counts. To test this,
I actually profiled the code in the ANTS Profiler, executing each method 1000000
times. The results showed no performance boost at all, rather a slight decrease
in performance:
Original: 12.2521 seconds
Rewritten:
12.8333 seconds
Amazing!
Or is it? When you look at what the
code is actually doing, the original version is going to have 3-5 of its lines executed
depending on the value of productStartAdded and filterCount.
The rewritten version will always have all three of its lines executed. So the performance metrics that we obtained
now make sense.
So now that we have painstakenly shown that, even though
you think it will impress your friends, rewriting the 19 lines of code into three
lines doesn’t help performance a bit.
What should we then conclude about this result?
The conclusion that I take away from this is that, since
the performance is basically the same in both cases, I would rather see the original
19 lines of code that I can read and determine easily what the purpose of the code
is, versus seeing the rewritten three lines of code and sitting there scratching
my head for an hour trying to decipher these cryptic statements.
So would your fellow developers who are going to maintain your code.
Read the complete post at http://www.dotnetmafia.com/Home/tabid/37/EntryID/61/Default.aspx