The many benefits of code reviews, and how to achieve them – 3. Improving the Process
8 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 discuss improving the interaction between the reviewer and the recipient of the review (i.e. the developer).
How do we mediate reviews?
There are all sorts of ways to mediate a review:
- Specialist review tools like Crucible and UpSource
- Repository systems with review features, like GitHub, GitLab, and Gerrit
- Sending diffs/patches over email
- Talking through the code face-to-face, e.g.
- Have the reviewer sit with the developer at their computer
- Reviewing code as a team around a big screen
- The old-fashioned way: Print out the code, stick it to the wall, and scribble over it together
- Pair-programming is arguably a form of code review, taking place as the code is written
Broadly speaking, all of the above approaches fall into two categories: asynchronous online reviews, or in-person reviews. Which kind of approach people preferred was probably the most contentious part of our discussions at Softwire. There was a range of opinions here, highlighting benefits and drawbacks to each approach.
Asynchronous online reviews vs. in-person reviews
Several people made the point that in-person reviews can be more useful for training, and for knowledge sharing in both directions. Face-to-face discussions make it easier to provide context around the code changes. They also give the developer a chance to talk the reviewer through the changes in a sensible order, or perhaps commit-by-commit. This might be better than the arbitrary order in which changes are presented by a diff/patch or an online review tool.
In-person reviews may also provide opportunities to pick up other context that might not be directly relevant to the code quality but is useful for the reviewer to know. For example, any frustrating obstacles the developer encountered while working on the task, which the team might need to address. Reviewing in-person can also save developers from context-switching. If you have enough reviewers on a team, developers can get a review as soon as they finish their code rather than starting on another task and subsequently switching back to deal with review feedback. This obviously comes at the cost of the reviewers having to make themselves highly interruptable though.
A lot of the literature on code reviews also favours some kind of in-person reviews. Here’s one particularly strongly stated example:
“Effective code reviews are not done via a tool or remotely—they are done when you’re sitting side-by-side with the person or pair who just wrote the code. This personal way allows you to share and teach much more information than you can pass in a text-based tool. Don’t skimp on this! If you’re going to do code reviews because your code sucks, do them right.” – Roy Osherove in Notes to a Software Team Leader
On the other hand, some of our reviewers felt that asynchronous online reviews were better partly because they don’t provide the reviewer any additional context. Online reviews arguably make for a more authentic review of maintainability (future developers on the project probably won’t have the luxury of talking through the code with the original developer). Also, coming at the review from their own angle might allow the reviewer to spot issues that the developer has missed.
One major advantage of online tools is that they leave a permanent record of review comments. Some tooling combinations make it particularly easy to go back through old reviews (for example, Crucible with JIRA integration). Several people had worked on projects where they had benefited from the ability to do this.
Several people found it useful to mix online and in-person approaches, perhaps depending on the nature of the change. For example:
- Performing a high-level review in isolation first, then talking through with the developer for context, before finally performing a line-by-line review online.
- Saving face-to-face reviews for bigger or more complex changes
- Carrying out most of the review discussion in person, but using an online tool to track this. That is, initiating the review process and documenting any important outcomes of the discussion.
Reviewers: Making code reviews better for the developer
Quite a few people found phrasing review comments to be a challenge, especially when using online review tools. Some of our reviewers were concerned whether we did enough to make new-starters comfortable with the process, and to make it clear that they can and should challenge their reviewers. After all, the developer is always closest to the code and knows it best. It can be worth a reviewer (particularly one in a more senior position) reminding the developer of this explicitly.
Ways to make reviews more positive included:
- Phrasing review comments as questions or suggestions rather than statements
- Talking through major issues in person rather than writing lengthy review comments
- Talking through and fixing batches of very minor issues in person, rather than writing lots of tiny review comments
- Remembering to always make some positive comments (especially in reviews with some criticisms elsewhere)
This might be more or less important depending on the developer. Generally, reviewers should be conscious of the recipient of the code review and tailor things to them. Extra tact may be required when reviewing code from external third parties, which can be politically awkward.
Developers: Making code reviews better for the reviewer
This wasn’t a question we had set out to answer with our discussion. However, people naturally mentioned how they approached submitting their own work for review, and several common points arose:
- Performing their own review of the changes first (ideally in the same review tool the reviewer will be using)
- Linking to any relevant context (e.g. the relevant ticket in the project’s issue tracker)
- Keeping auto-generated files out of the review (if appropriate and the review tool allows)
- Splitting into sensible commits
- Especially keeping big renames, file moves, or other refactorings separate
- Also splitting code changes across multiple commits where appropriate)
- On one project, we experimented with commiting and reviewing one test at a time, resulting in many small reviews. On a small team, this turned out to be a very effective workflow.
As we saw in the previous post, there are many different valid approaches to code reviews. Organisations should give teams the flexibility to choose a code review process that meets their needs. The first post in this series covered the wide and varied benefits of code reviews. As a team, you should reflect on your code review process, considering what value it provides and what further value it could provide. This will allow you to evolve your code review process to be more effective. I hope this series gives you some ideas that you find useful. Please feel free to share your own ideas on code reviews in the comments below.