Code reviews have moved from a time-intensive, low-impact paper-based technique to a powerful and efficient tool as part of modern software engineering. This article explores the history and basics of code reviews, to see how the code review sits within a high-performing software engineering team and explores what techniques we can use to maximise their value and impact.

The Traditional Code Review

It has long been known that code inspection finds more defects than user testing, with this in mind formal code reviews were introduced in some organisations.

Such a review was an exercise where a senior developer would take a printout of a junior developer’s code, spend some time examining it, then sit down with the junior to note required changes; which the junior would dutifully go ahead and make. There would be varying levels of commitment from management and there are various problems with this approach:

• Hard to say what code was actually changed (a printout doesn’t say)
• Very much a Master-Apprentice relationship
• Often seen as a pointless administration exercise
• Very time-consuming and bureaucratic
• Difficult to see if changes have been done and rereview
• Impractical to have more than one reviewer
• Little transparency

The Code Review in an Agile Team

Nowadays we have tools to manage some of these issues. Typically with Github or Bitbucket the process would be:

• Developer branches the code for a particular feature
• Changes are committed to the feature branch
• When the feature is complete the developer raises a Pull Request(PR) and selects one or more senior developers to review
• The reviews can see exactly what was changed and can comment upon each change
• The developer can then make changes and resubmit the Pull Request
• Once accepted the code is merged into the “main” development branch

This approach improves massively upon the paper exercise. Management support this, as they have approved the tools, and developers like this as it is an almost seamless part of the flow of feature development; in fact to bypass this process requires deliberate effort.

Many teams will have rules around when a Pull Request may be merged, for example you may require that two senior developers must approve before the merge is done.

Approval Criteria

Many organisations will have a set of coding standards for development that include code layout, naming, architecture and database access. It is a sign that you do not have sufficient standards in place if your developers cannot agree on essentially non-functional issues like this. The rise of Convention over Configuration, as typified in Ruby on Rails, means developers have clear guidelines on approach.

Refining Approval Criteria

Coding Standards

A team should have a clear set of coding standards, for most languages there are community-defined standards and it is recommended you follow one of them. Coding standards should be checked during a code review, but automated tools are available that will address most issues before a manual review is started.

Architectural Standards

As part of the review, architectural standards must cover issues such as the use of third party libraries, logging, database access, Microservices and Microservice clients. Use of Convention over Configuration helps massively in this area. Guidelines on premature optimisation are also essential.

Definition of Done

A successful Agile team will have a strong Definition of Done (DoD), as part of the code review the reviewers can check things that contribute towards DoD when the story is complete. For example, tests are a universal requirement.

Maintainability and Readability

Can we see any issue with extending this code? Some organisations like to ask ‘can this code last X years’? Code readability is a subject all of its own, but in short:

• Do the variable, class and other names make sense? Can we make them shorter? Should we make them longer to be more explicit? Confusion around what something is called can often be a sign of poor design
• Use of decomposition, layout and comments to clarify intent
• If an uncommon technique is used (e.g. a mathematical algorithm) you should include a link to how it works (a link to Wikipedia is often sufficient)

Maximising the Value of a Code Review

Subvert the Roles

Traditionally, within the context of code reviews, a senior developer has a Master-Apprentice relationship with the junior developer. However to maximise the value of code reviews we may insist that within the context of a code review, both roles are equivalent. Every code review must be done by at least one junior and one senior developer; this affords that:

• Everyone’s code is subject to the same scrutiny. The code is under the microscope, not the developer
• The junior sees how a senior approaches a problem
• The junior can correct a senior
• It fosters a culture that correctness is more important than seniority

It should also be expected that anyone can take part in a code review if they wish to do so, even if the developer has not selected them as a reviewer.

No Comment Left Unanswered

It’s essential that any comment anyone puts against a Pull Review is answered, if not it allows the assumption that we can pay lip service to the review process. It also sends a clear message to the commenter that what they have to say is of no consequence.

Handling Conflicts

In a mature Agile team there’s a focus on correctness and team learning, not individual heroics or “force of will” to get things done. If we leave our egos at the door and have a sense of humour most conflicts will resolve themselves within the team. It is a healthy sign if others are invited into the code review to discuss an issue.

At some point there may be an impasse, at this point the team lead/architect will step in and make a decision. A great leader will make handle the conflict and ensure everyone learns from it.

Be Open

To get the most out of the code review each person taking part in the review should be open, honest and accept they may be wrong. Doing so includes “not sure” comments, and asking open questions.

Ownership of the Code Review

The code in a Pull Review and the code review process for that Pull Review is owned by the original developer. It is their responsibility to ensure reviewers do a thorough job of reviewing and that all comments are acted on.

Checklist

To ensure we’re maximising the value of code reviews:

• Does management buy into the idea of code reviews?
• Do developers value code reviews?
• Do we use integrated tools such as GitHub or Bitbucket?
• Do we have clear coding and architectural standards?
• Do we check against Definition of Done?
• Do we take action if we see issues recurring in reviews?
• Do we ensure all developers have the same seniority within the context of the review?
• Can any developer join a code review?
• Does the original developer own the review?
• Are all review comments answered?
• Are comments short and clear with minimal room for interpretation?
• Do we have an environment that fosters openness and honesty?

Summary

The value of code reviews has been known for a long time, but historically the lack of tools has made this value harder to achieve. With the use of powerful and integrated tools the code review is now at the heart of delivering functionality. Here we have taken common industry practice and outline steps to maximise the value of a code review so we may improve the quality of code and the knowledge and practice of all developers.