This is the 5th part of a series about 67 Bricks’s coding principles. The previous posts are: 1, 2, 3 and 4.
The principle
All code should be reviewed before it is merged into the main branch. All project members, however junior, should be involved in code reviewing.
This is another – I hope – uncontroversial principle.
At 67 Bricks we generally use Gitlab for our code hosting and use their merge request feature for signalling that a change is ready to review and then for checking the changes and leaving comments for the author. All the other big git hosting platforms have equivalent tools that are just as good, so there’s no excuse not to do code reviews when working in a team.
Code reviewing is beneficial for the quality of the codebase because a reviewer may spot edge cases, mistakes, issues or potential problems that the original author didn’t consider. They may also be able to suggest improvements, based on their own knowledge of the project or of the relevant technologies.
We all make mistakes, so having another pair of eyes looking over your work makes it more likely that those mistakes get noticed before they cause a real problem. We also all have different knowledge, experiences, strengths and weaknesses, so code reviewing is a way of bringing the experience of a second person to bear on a problem.
Another benefit is that the reviewer comes to the code at some distance from the detailed problems the developer had to wrangle with, and this can be useful when seeing the complete set of changes as a whole. This is distance that the author will probably gain themselves over the next days and weeks, but it’s useful to have it immediately from another person.
Knowing that your code will be reviewed also encourages you to be more thorough. This is just human behaviour, especially when we are busy and keen to be seen to be making progress.
Slightly less obviously, code reviewing also has benefits for the reviewer because it exposes them to areas of the codebase they may not have worked on before and encourages them to engage with, and constructively criticise, others’ code. This gives them the opportunity to be exposed to and learn from others’ approaches.
And I should emphasise constructive criticism. When it works well, code reviewing can lead to a closer team built on trust. When we, as reviewers, suggest changes, we need to do so without implying criticism. And when receiving review comments, we need to understand that the health of the codebase (and the project) is more important than our egos.
As much as it takes effort to do a good, thorough code review, the benefit is huge. I’m sure I can’t be the only person who has been guilty of waving through a review with minimal attention – perhaps because I know the author is sensible and writes good code – only to find later that it caused some bug that I could have prevented if I’d engaged with it a bit more. Skimping on the effort to review properly is a false economy because the mistakes you miss will just need to be fixed later, leading to more reviews.
Generally speaking we at 67 Bricks think only one person need review each change, but there may be cases where it makes sense for more than one person to be involved, for example to get the input of a particular subject matter expert.
I don’t think anyone would pretend that reviewing code changes is their favourite part of the job, but there are things we can do when putting our code up for review that make everyone’s lives easier.
- we can aim to keep merge requests small and focussed (spoiler alert: this is the focus of a future principle)
- we can provide any necessary context, descriptions and (if applicable) screenshots when opening the merge request to give the reviewer the best chance of understanding what they’re looking at
- we can aim to keep each commit focussed on a single, meaningful change rather than lumping lots of unrelated changes in each commit. This makes it easier to review commit by commit, which can be preferable in some cases
- we can be available to answer the reviewer’s questions. It can even be helpful to quickly walk a reviewer through your change so they fully understand the context and your intentions before fully reviewing it themselves
Code reviews can unfortunately lead to a bottleneck in the development process where a number of changes sit unreviewed and becoming stale while the team works on other things, so it’s worth trying to keep on top of them. It often works to have a team policy that, upon finishing a piece of work, members should review at least one open merge request before picking up something new.
Code reviewing generally isn’t what gets anyone up in the morning, but it’s immeasurably valuable for the overall quality of the codebase. And slacking on it is likely to lead to costlier problems later on, so it’s worth trying to do well.