Code Reviews

Code reviews should always be part of the development process. Code reviews are an efficient way of finding defects in software.

I have come across programmers who resist code reviews by claiming that they do not have the time to do code reviews. I have an instinctive response to such complaints: "You do not have time because you are so busy finding the cause of bugs in your un-reviewed code." What I actually say is closer to "I think you will find that code reviews will save you time in the medium-term, because there will be fewer bugs found by QA and in Production, so you will spend less time chasing down bugs." To backup that statement I point developers at McConnell's Code Complete, which quotes research on the effectiveness of code reviews.

I have seen teams use both peer reviews and formal reviews. Peer reviews have a lower impact on an engineer's schedule. The reviewer can study the code at a time that is convenient for them, rather than at a scheduled meeting. On reflection, I am convinced that this advantage is more than outweighed by what is gained from a more formal review.

It is difficult to ensure that engineers are investing sufficient effort in peer code reviews; at least until bugs get into production, when it is too late. With a formal code review, the participants are in a meeting and committed to taking the time to actually read and think about the code.

Having a developer explain their code is enlightening, sadly often in a disheartening way. When the developer cannot recall their reasoning for the code they wrote a few days previously, it does not give one faith in how understandable the change will be in a few months. When this happens, I can rarely resist recounting the aphorism "Write your code assuming that the person maintaining it is an axe wielding psychopath who knows your home address."

Formal reviews are good learning exercises. The reviewers see code from other parts of the system, participants see other ways of writing code, and everyone learns from observations about how to improve the code under review. Whether those lessons are taken as intended is another question. One of my pet peeves is the use of "Magic Numbers":
Status = 9
Is a lot less clear than creating a constant and typing
Status = SOLD
I will always raise this as a suggested improvement in a review. On one such occasion, the coder's response was that she had used the "magic number" because she did not know I was going to be in the code-review. This is the equivalent of rolling through a STOP sign when there is no police officer.

In McConnell’s aforementioned book, he warns against using code-reviews as input to appraisals of engineers. This is a rule, like never driving above the speed-limit, that is hard to stick to the exact letter. I have not, and never would, use measures based on the number of changes made in a code review as part of an engineer’s target. Nonetheless, the quality of code produced is an important part of a developer’s work. Sometimes in a code-review, the quality, or rather the lack thereof has been too severe for me not to take action. One occasion sticks very firmly in my mind. I was in a code review for a newly hired engineer. He had a full screen of code that served just a single purpose; to convert an mm/dd/yy date into dd/mm/yy. It was so complex that it was not apparent how or even if it worked. That was a bad sign, but frankly the lack of consideration of the the following three points was more worrisome:
a) Why did the developer not recognize that this was too complex
b) Why did the developer not look for code already in the codebase that did this format switch
c) Why did the engineer not look to see if the language supported native date format switches.

That engineer did not last long with us.

Code reviews find defects that can be hard to locate with functional testing; especially around exception handling and record locks. Code reviews help spread knowledge of the system. Code reviews, by definition, find the line of code that is wrong; developers can often spend significant amount of time finding the code responsible for a defect in QA or production. Code reviews are meetings that are productive. Every code change should be reviewed.