The many benefits of code reviews, and how to achieve them – 2. How do we go about code reviews?
1 February 2017, by Harry Cummings
This is part of a series of blog posts on code reviews, based on two sessions of an internal discussion forum at Softwire. See the first post in this series for more information. In this post, we’ll cover some of our current approaches to code reviews.
We tend to at least implicitly perform code reviews in multiple passes. These break down into three stages:
- “Outside-in” preliminary review
- Reading through the original user story or defect
- Reviewing the design
- Checking out the dev branch and doing some cursory testing (this can be useful for reviewing UI issues or things that are hard to spot from the code or by automated tests)
- Reviewing the tests at a high level (do they function as good developer documentation for the code)
- Review of the code itself and the tests in detail
- Review of any activities surrounding the code change, e.g.:
- Manual testing
- External documentation
- Risk/impact assessment
Note that not every project needs all of these passes. The point is that “code review” is a broad term covering a range of activities. Which activities you carry out, and when, may vary by project. Although within each project, there’s a lot of value in being consistent. Consistency helps developers become comfortable with the review process, and makes code reviews a much more reliable tool for quality assurance.
When do we review
As noted above, different code review activities may be carried out at different times. There was a general consensus in our discussions that reviewing earlier is preferable. Most projects insisted on at least some form of review before commit, although a few relaxed this in special cases (depending on the type of project) to avoid becoming a bottleneck.
About half our teams are actively performing up-front High-Level Design reviews. These can be useful for everyone but especially for less experienced developers (which might just mean less experience with the particular project). They encourage working through design issues up front, avoiding wasted time at the implementation stage. It also means the code reviews can then focus on just the code. The only problem mentioned with HLD reviews was that it can be a bit unclear what we mean by an HLD, and sometimes people go too low-level. For projects broken up into well-sized tasks, an HLD could just be a couple of sentences and a few bullet points.
An alternative to up-front HLD reviews is reviewing roughly implemented code, essentially a spike or proof-of-concept. This can be particularly useful on tricky legacy codebases, where it might be hard to see how to go about introducing new functionality.
Who carries out reviews?
Most of the people in our discussions were their team’s technical lead. Unsurprisingly, tech leads were doing reviews themselves, but there was a lot of support for reviews being done by not only the tech lead. Getting more people involved in the review process is a good way to build people’s confidence, share knowledge within the team, and help people become more comfortable with the review process. One person doing all the reviews can also become a bottleneck and slow the team down. Perhaps more importantly, giving developers the autonomy to carry out genuine peer reviews is a show of faith in the team’s ability, and makes it easier for reviews to act as a positive motivator.
One problem with having multiple people involved in reviewing is that it can become confusing for developers. It’s not always clear how to pick an initial reviewer, or when a review would be considered “done”. It’s important for each team to agree on a consistent approach, although approaches can of course vary between teams. Most of our teams use one of the following approaches:
- Let the developer choose the initial reviewer and allow the developer or the reviewer escalate to the tech lead if needed
- Have a high-level second line review as part of the standard review process
- Include the tech lead on every review, but allow developers to merge their changes as soon as at least one person has reviewed it. This prevents the tech lead becoming a bottleneck but still gives them a chance to go into detail on any red flags.
All the above approaches include the possibility of the tech lead acting as a second line reviewer. Our tech leads would go into more or less detail in their review based on the nature of the change and the experience of the other people involved (i.e. the original developer and reviewer). In some cases perhaps just reviewing the comments from the first-line reviewer and/or looking out for changes within common problem areas of the codebase.
How much detail to go into in a second line review is a matter of judgement and may not be obvious. It can help to think of the goal of reviewing as gaining trust that the code is up to standard, and getting involved enough to meet this goal. Of course, it’s still worth bearing in mind the importance of code reviews for training and mentoring. A second-line reviewer may be looking out for learning opportunities for both the developer and the initial reviewer. They’re also in a position to assess the quality of the interaction between these two roles. This will be the subject of the next post.