The Art and Science of Code Reviews

Posted on Tuesday, October 30th, 2012

Improving the Effectiveness of Development Teams and the Quality of the Product They Produce

At Seva, our success depends on knowledge sharing. Like any consultancy, the chief benefit we offer to our clients and partners is a commitment that they are not just receiving the benefit of one consultant’s expertise or even the cumulative knowledge of an assigned team. Instead, we bring the wealth of experience accumulated by the group as a whole to each engagement. To fulfill this promise, internal knowledge sharing within our group is key.  Regular code review is one of the best tools available for this purpose.

The Types of Knowledge Gaps

There are really two kinds of knowledge gaps that software engineers and developers encounter on a regular basis: blocking and non-blocking. Certainly Seva is no exception.

Blocking knowledge gaps are the blind spots that prevent a consultant from finishing a specific task. The missing piece of information in these cases is often easy to specify and search for on the Internet or in documentation. Consequently, a sea of websites exists to handle that demand.. Thus, the knowledge sharing that is required when a consultant is blocked is often already occurring via email, searching the web, or just meeting in the hallway. When you’re blocked, you look or ask until you find a way to move forward.

Non-blocking gaps are altogether different. These are the “unknown unknowns”; the types of things that move a solution beyond minimally sufficient into other ideal forms such as elegant, robust, and scalable.

To give one example from the literature: In Josh Bloch’s book Effective Java, he identifies a situation in software construction where a developer has to make a decision: what should I do when my program is asked for a list of objects that aren’t available?

The two possible answers are a) to return null and b) an empty list. His recommendation is to do the latter, as this obviates the need for one type of error checking and eliminates the possibility of a ghastly null-related system crash that will eventually ruin someone’s day.

Writing that method to return null (the wrong way, mind you) will almost certainly work well enough, initially. It isn’t rare in software to write two code paths, the first of which is hit 999 times out of a thousand. However, it is precisely the fact that it works so often that this defect (and it is a defect) made it into the system. No single person was blocked on it, and thus there was no reason to go out and look for that “better way”. The text concludes:

“[Returning null in this case] is error-prone, because the programmer writing the client might forget to write the special-case code to handle a null return. Such an error may go unnoticed for years, as such methods usually return one or more objects. “

It’s often said that there’s a difference between having ten years of experience and having one year of experience ten times. What you’re looking for when hiring for the former case is the steady accretion of knowledge like the above; the person who not only knows how to do the simple things, but can also avoid the subtle traps.

This is the type of information that is critical to share across a software team, as it not only improves the software being written but also the team members themselves. Unfortunately, since most often this type of information is only useful in these “non-blocking” scenarios, there is rarely the necessary context to bring it up. With regular code review, that situation changes.

Code Reviews

Atul Gawande, both a surgeon and journalist, recently wrote an article for the New Yorker asking a simple question: why do elite athletes use coaches while most other professions do not.

Élite performers, researchers say, must engage in “deliberate practice”—sustained, mindful efforts to develop the full range of abilities that success requires. You have to work at what you’re not good at. In theory, people can do this themselves. But most people do not know where to start or how to proceed. Expertise, as the formula goes, requires going from unconscious incompetence to conscious incompetence to conscious competence and finally to unconscious competence. The coach provides the outside eyes and ears, and makes you aware of where you’re falling short. This is tricky. Human beings resist exposure and critique; our brains are well defended. So coaches use a variety of approaches—showing what other, respected colleagues do, for instance, or reviewing videos of the subject’s performance. The most common, however, is just conversation.

As a craft, software development is no different than what he is describing above. In our case, a mixed team is its own coach, pointing out misunderstandings, errors, or just alternatives to consider; each to another as the situation dictates. The code review is the conversation.

At Seva, we’ve found code review to be very beneficial in raising the standards and skill levels of everyone. Among the benefits:

• More readable code: Regular code reviews make code readability one of the minimum standards applied during software construction. It moves from a long-term, “if there’s time” benefit to a short-term requirement. Since the code will be read before it counts as “done”, readability (and thus, maintainability) is never cut for time.

• Better tested code: One of the requirements for each review request is that the person who wrote the code must describe the testing that was done. This isn’t to keep people honest per se, but rather a deliberate exercise in forcing the developer to think through the types of cases and conditions that affect the change he or she is making. Often, one or two edge cases are found this way before anyone else has to look at it.

• Less duplication: Described by Joshua Kerievsky as “the most pervasive and pungent smelling software”, duplicated code is an increase in maintenance without any increase in benefit. More eyes on the code provides an opportunity for a developer to spot and eliminate these duplications as they come in. On large projects, each developer has incomplete knowledge of the system. Consequently, the group-wide review is the best tool for catching this type of mistake.

• Cross-training: The explicit knowledge sharing already mentioned. For the person who wrote the code, it is an opportunity to share any lessons learned. From the reviewer’s perspective, it is an opportunity to correct any explicit bugs and to provide more nuanced guidance if necessary, filling those ‘non blocking’ knowledge gaps for the author and the rest of the team. This can be a very effective way to train junior level employees or even senior level employees that are new to a particular project and business domain.

Note that code reviews need not be overly structured. On a recent project, for instance, we did not have the need for a “review manager” or any explicit role related to this activity. Each member contributed their expertise when applicable. In some cases, that involved correcting malformed code or making suggestions for better performance and clarity. In others, it was to the need to identify a mismatch between the implementation and the understanding of the end-user, closing a potentially expensive expectation gap. Either way, it was an occasion for a public exchange in a way that benefited everyone.

The Cost

Closing these “non-blocking” knowledge gaps used to be “brown bag seminar” fodder, with PowerPoint, lunch, and so forth. Not only is this inefficient from a time perspective, as a lunch session involves too many people, takes up too much time, and is usually too general, but it is also disruptive to the typical developer’s schedule, as it breaks up the day into two less usable parts. Code reviews are much cheaper. A code review “training session” only happens when necessary, involving only the people that are necessary.

The majority of our reviews at Seva are simple approvals that take less than two minutes. If there’s a problem, the review takes longer, but right now is when you want to have the problem. Code reviews are especially effective at rooting out many different classes of defects, and surfacing them here is a good thing, as it is much cheaper to fix an issue this early in the construction process. Not only that, formal reviews can train the whole team to avoid the error entirely which is, for the next project, the cheapest solution of all.

Download This Seva Perspective

Download the PDF version