They say brevity is the soul of wit and in a code review last week I came across some code that was definitely not funny.
The idea was to use JavaScript to enable a button if and only if two textboxes had been filled in.
myButton.disabled = false;
if ( firstTextbox.value.length == 0)
myButton.disabled = true;
if ( secondTextbox.value.length == 0 )
myButton.disabled = true;
The code does its job, but you have to read five lines to figure out what that job is. That's bad, but it became worse when a developer wanted to add an additional consideration. We decided that we also wanted to disable the button if the second textbox was completely filled with spaces. The developer maintained the code in its original style:
myButton.disabled = false;
if ( firstTextbox.value.length == 0)
myButton.disabled = true;
if ( secondTextbox.value.length == 0 )
myButton.disabled = true;
else if ( secondTextbox.value.replace(/ /g, '').length == 0 )
myButton.disabled = true;
Now the middle test is unnecessary, but that was not obvious to the developer because it was expressed in so wordy a fashion that he did not pay enough attention to it. I mean, who wants to look at code like that!?
If the original code had been...
myButton.disabled = firstTextbox.value.length==0
|| secondTextbox.value.length==0;
...then chances are that the change, too, would have been made in better style:
myButton.disabled = firstTextbox.value.length==0
|| secondTextbox.value.replace(/ /g, '').length==0;
So, seven lines of code would have been one and a half.
We developers have heard over and over again that we should not try to jam too much logic into too few lines of code. As a result, our coding style is sometimes plodding. As our code is maintained, it becomes even more ponderous. Worse, it becomes difficult to apprehend at a glance, so it's easy to make mistakes when changing it.
If I have not convinced you, then maybe Visual Studio will. I fed the following C# code into Visual Studio 2010's code analyzer to get the cyclomatic complexity of each routine.
static bool WordyEvaluation(string a, string b)
{
bool ret = false;
if (a == "a")
ret = true;
else if (b == "b")
ret = true;
return ret;
}
static bool ConciseEvaluation(string a, string b)
{
return a == "a" || b == "b";
}
The cyclomatic complexity of the wordy code was 3. The concise version rated only a 2. Visual Studio's maintainability index for the concise version was also significantly better (83 vs 68).
So remember: brevity is the soul of maintainability!