public void CheckLimitsWithinRange()
{
CheckInRange(AbsoluteMinimum, SuggestedMinimum, "absolute minimum", "suggested minimum");
CheckInRange(SuggestedMinimum, SuggestedMaximum, "suggested minimum", "suggested maximum");
CheckInRange(SuggestedMaximum, AbsoluteMaximum, "suggested maximum", "absolute maximum");
CheckInRange(AbsoluteMaximum, 9999, "absolute maximum", "9999");
}
private void CheckInRange(decimal first, decimal second, string property1, string property2)
{
if ((first < 0) || first > second)
Errors.Add(String.Format("{0} cannot be less than 0 or greater than {1}", property1, property2));
}
and compare that with
public void CheckLimitsWithinRange()
{
if ((AbsoluteMinimum < 0) || (AbsoluteMinimum > SuggestedMinimum))
Errors.Add("absolute minimum cannot be less than 0 or greater than suggested minimum");
if ((SuggestedMinimum < 0) || (SuggestedMinimum > SuggestedMaximum))
Errors.Add("suggested minimum cannot be less than 0 or greater than suggested maximum");
if ((SuggestedMaximum < 0) || (SuggestedMaximum > AbsoluteMaximum))
Errors.Add("suggested maximum cannot be less than 0 or greater than absolute maximum");
if ((AbsoluteMaximum < 0) || (AbsoluteMaximum > 9999))
Errors.Add("absolute maximum cannot be less than 0 or greater than 9999");
}
The two blocks do roughly the same validation, but in my opinion, the latter is far more readable and self-documenting than the former. Yet, I've seen many developers shy away from the easier but plainer code, because the developer instinct often compels them to. The beginner looking block of code is also more maintainable. Imagine part of the rule changes to "suggested minimum" has to be less than 75% of the "suggested maximum". The first example will require a new programmer to retrace the exact thought process of the original writer when that code was first conceived, which is more costly in terms of the time spent.
The example I gave seems pretty silly, but the underlying symptom is very common. When applied on a larger scale, the maintenance overhead is no longer measured in seconds or minutes but rather hours lost in project resources. I guess the point of this rant is that there's a fine balance between well-engineered code and over-engineered code. Not every block of code needs to match the consulting fee your client is paying. It's OK for code to look cheap sometimes.
Posts
Strange.
ReplyDeleteI thought you were going to say that the first block was better. To me the first is more readable than the second. If the number of checks increased further I would say that the first would become even more readable by comparison to the second.
Perhaps the example is too simplistic. In the first example I can see immediately that you are doing range comparisons due to good naming of the method. In the second I find it takes longer to check each element to see that each is such a comparison.
I would also suggest that the first is better (not just more readable) because of the code reuse. There is much more likelihood of an error being introduced into the second piece of code. Also, if the logic for the comparison changes, you would only have one line to fix.
Not counting the method declarations, the first block even achieves the goal using less lines of code....
ReplyDeleteI would agree with BlackWasp. The former code black is easier to review IMHO. Additionally, it is easier to maintain & reuse (i.e. The process logic is separated from the utility of validating the range). If the code becomes more complex, the latter code block could become the less superior of the two examples.
ReplyDeleteIf the static strings is the source of the concern around readability, then they could be exported to resource files and their representations used here (e.g. CheckInRange(AbsoluteMinimum, SuggestedMinimum, MsgToUser_AbsoluteMin,... ). This makes translation easier, and I often find that I do not need to know what the user message is unless I am debugging for that specific error message.
First remember that this is all very subjective, and readability really depends on each person's coding style.
ReplyDeleteBlackWasp and Sheldon, there are a couple assertions in the comments that I don't necessarily agree with. In example 1, knowing that I am performing some kind of range checking is less important in the context of business requirements than knowing immediately what's range I'm validating against. Example 2 is actually quite literally how the requirements would typically be stated in the document. There we have self-documenting code.
Also the code reuse here would potentially hurt maintainability. You are assuming that a future change will be applied uniformly, which is often not the case. Real business requirements are typically more complex. Imagine that we want to update a requirement to "suggested minimum" has to be less than 75% of the "suggested maximum", and then add "absolute minimum" also has to be less than 50% of the "absolute maximum", which is more likely the type of changes you get from the product owner.
If I give this change request to a new developer who never looked at the code. With example 2, he should be able to identify immediately what is currently implemented as if he was reading through a document and then find the appropriate line to change or add the new calculation.
In example 1, he would have to first understand what the helper function does; understand how to use the parameters; why there are 4; which check-in-range call to modify; decide whether the new calculation should be placed inside or outside of the helper function; how to update the error message accordingly; and possibly realize that the string parameters also need to be renamed. I would think that's a bit involved.
I am definitely not advocating against code reuse in general. But remember, reuse introduces complexity. And sometimes, simpler is better, even if it's a bit more redundant. In this case, I much rather have the rules themselves stated as explicitly in code as possible, free of the noise added by the programming language constructs.
Anonymous, if your goal is to have a lower LOC count regardless of the context, then you've missed the point of this post entirely.
As I say, perhaps the examples are too simplistic. I certainly agree with your premise that simple is better though. One should always consider maintainability and readability over size and cleverness.
ReplyDeleteSome of my worst moments working were on some software that the previous developer thought it would be good to create one master data access method to centralise all database queries and updates (that's right, one method for both). His simple method required about fifteen arguments, many of which were classes containing complex arrangements of data.
Those were the days :)