Code In Review

Let me start by saying that reading code is rarely fun.  Understanding it is hard.  Let’s assume that is a given, and avoid all the arguments that surround that topic.

Unfortunately, if you are a software developer, reading and comprehension skills are critical to your professional success.  Maintaining legacy code, integrating third-party components, even simple teamwork requires a developer to read and comprehend the execution of code written by a different developer.  Most real-world projects are simply too large to exist solely in the hands of a single developer, and if it did, the risk to that project would be so large as to be untenable for a responsible company.

Given the seemingly critical nature of reading and understanding code written by other people, why are we content to dismiss it as hard?  Writing software is just as hard, but no one seems to wave their hand at that problem as too difficult.  Not only that, but we talk about writing code for others, but never seem to discuss reading other people’s code (try searching Google for “writing readable code” and compare it to the results for “Reading Code”).

This seems to be a fundamental deficiency in the training of future software developers.  When we learn to write, we do not just sit down and memorize the rules for sentence construction along with word definitions.  Instead we balance those grammar rules by reading the works of authors who have already mastered their craft.  We read books, newspapers, magazines, journals, and develop an ability to recognize “good” and “bad” writing, independently of the actual validity and specifics of the sentence and paragraph construction.

Why can’t we do the same thing in software?

I’ve already talked about differentiating yourself by becoming comfortable in a domain outside of the development world, but that doesn’t always translate to your next project or job.  So if you want to differentiate yourself in the software development world, learn to read code.  Learn to recognize well-written code and poorly written code.  Learn how other people approach problems and construct solutions.  Your own work will improve immensely as you learn to recognize your own good and bad designs.

Quick question, what is wrong with the following code?

bool result = false;

if (test1) {

  if (subtest1) {

    DoSomething();

  } else

    DoSomethingElse();

  result = FinalMethod();

} else if (test2) {

  result = GetSecondResult();

}

return result;

Stumped?  What if I told you that FinalMethod should only be called if “subtest1” is false?

I have changed the names and circumstances to protect the innocent, but the facts of the error remains the same as one i just encountered in a code review.  Of course, even knowing that little tidbit of information, it still took me a couple of minutes to see the error (Fortunately my spidey sense was tingling, so I knew *something* was wrong…)  The first time I read the code, this is what I saw (I reformatted to make the misperception clear, but the formatting was in reality the same as above):

bool result = false;

if (test1) {

  if (subtest1) {

    DoSomething();

  } else {

    DoSomethingElse();

    result = FinalMethod();

  } else if (test2) {

    result = GetSecondResult();

  }

}

return result;

Of course, that is a nonsensical statement, but that is how my brain interpreted the block.  Since the code was missing the brackets around the else statement, my brain simply supplied the expected syntax, regardless of its validity.  Given that the error was an introduced error, it is safe to assume the original developer’s brain did much the same thing, since there was even a comment to the effect that if subtest1 failed it would return the result of FinalMethod().

So how can we prevent these types of errors?  If our brains are actively working against us, then we need to work to give our brain what it wants:

bool result = false;

if (test1) {

  if (subtest1) {

    DoSomething();

  } else {

    DoSomethingElse();

    result = FinalMethod();

  }

} else if (test2) {

  result = GetSecondResult();

}

return result;

Now there is no need for our brain to supply the missing brackets, since they are explicit.  In addition, if we had originally used those brackets, it would have been immediately clear to anyone reading the code that the call to FinalMethod was occurring outside the else block, making the error much easier to detect, and much more unlikely to enter the codebase.