The Code Review Secrets of High Performance Teams
There is a common misconception in many software organisations that development teams can either focus on shipping features or maintaining a high-quality codebase - but that you can't do both at the same time. This simply isn't true.
Whilst it may be the case that development schedules often militate against focussing on quality, by following an effective set of software development practices, you can avoid the pitfalls that all too often lead to the creation of technical debt and spending huge amounts of time having to constantly fix bugs. Focussing on quality means that we can steer clear of these problems in the first place, allowing for this previously wasted time to be invested in working on the features that your customers want.
How can this be achieved? The secret to maintaining a strong release cadence whilst also developing a high quality codebase is a code review process geared towards supporting two metrics simultaneously - high quality code, and high developer productivity.
I have worked on a wide variety of teams during my career, and the most effective of those teams all shared something in common - their code review process. Today I will be sharing the key elements for code review that, when followed, maximise the team's productivity and go a long way towards ensuring a high quality codebase.
Prologue - Why code review?
I'm assuming if you're reading an article about code reviews then you're already sold on the general idea, but in case not, here are some commonly understood benefits of performing code reviews:
Catching bugs before they ship - it's far better to find bugs yourself before your users do, so any opportunity to do so shouldn't be overlooked. The more pairs of experienced eyes on a piece of code, the more likely it is that problems will be spotted sooner rather than later.
Knowledge transfer to reviewers - giving other developers an opportunity to review the code you're writing gives them a direct insight in to the workings of parts of the codebase that they didn't build themselves.
Knowledge transfer to the reviewee - if a developer reviewing the code knows of a better way to solve a problem than is presented in the code being reviewed, they can share it before the code is committed to the codebase. This is of particular benefit when mentoring junior team members.
Maintaining consistency in system design - everyone has their favourite design patterns, but mixing them all together in a single codebase is a recipe for disaster. Code review helps enforce consistency, aiding the creation of a uniform, familiar codebase.
How do code reviews increase code quality?
Quality improvements primary come from having more eyes on a problem and its solution, making it more likely that the best solution is the one that ends up being implemented. More experienced members of the team can also mentor junior team members directly at the code level, helping to improve the skills of the junior member. This levelling-up of skills has a compounding effect on quality over time. Higher quality code contains fewer bugs and doesn't contribute to technical debt as the correct solution to a problem is more likely to be implemented the first time around.
How do code reviews increase developer productivity?
If you ignore the code review process entirely, will you be able to ship features quicker? Yes you will. Initially. But once the list of bugs starts to pile up your team's velocity will drop due to having to context switch constantly on to bug fixing, and also through a drop in the team's morale (no-one likes to constantly be reminded of the things they've got wrong). The improvements in the quality of the codebase brought about by code review eventually leads to fewer bugs and less technical debt, freeing your team to focus on feature development instead.
OK, hopefully by now you're sold on the idea. But how do we go about implementing effective code review practices? We do it best by accepting a set of responsibilities that apply to both code reviewers, and code reviewees.
The Responsibilities of Code Reviewers
You've been asked to review a pull request. What should you do next?
Address the pull request immediately - the most important responsibility that you have as a developer, when asked to review a piece of code, is promptness. A code review is not something you do when you feel like you have the time, or when you've finished what you're currently working on, or after lunch, or maybe tomorrow if it's raining and you haven't got anything better to do. It is something you do immediately, as soon as it is requested. Why? Put simply, code that you have been asked to review is potentially finished and ready to ship. At least one developer (the one who wrote the code) believes the code is ready, otherwise they wouldn't be sending it for review. Whatever code you are currently working on is not yet in this state, as if it was you would have sent it for review already. Potentially finished code takes higher priority than incomplete code, by virtue of its possible completeness, and should be the primary focus of your attention. If you cannot take care of a code review immediately, you should let the reviewee know straight away, so that they can decide whether they wish to wait for you to perform the review, or take the opportunity to ask somebody else to do it.
By taking this approach to reviewing code we ensure that developers do not get blocked waiting for their code to be reviewed. The reviewee receives prompt feedback from the reviewer(s), which can either be to make changes to the code, or can be approval of their work, allowing it to be merged swiftly and unblocking the developer to begin their next task.
Check for unit tests - the next most important responsibility of the reviewer is to check for unit tests. The presence of unit tests never needs to be explained, but their absence certainly does. It may be that in some cases, such as making small changes to the user interface of an application, that no unit tests are required. But this is the exception and not the rule. Not only should tests be present, but reviewers must also check the completeness of the tests. Do they really exercise all of the public API of the application code? Does the implementation rely on exposing what should be private details in order to allow adequate testing (a fairly good indicator of poor API design)? The benefits of unit testing are numerous. When it comes to maintaining code quality, full test coverage is the best way to ensure that code that has been written actually does what the developer intends it to. This in turn reduces the possibility of introducing bugs that will have to be fixed later, which increases the time available for working on features in the future.
Do not check for formatting or code style violations - these are jobs for formatters and linters and should be automated as part of your build process. Your job as a reviewer is to do what cannot be automated, which is to understand the problem that the code is attempting to solve and provide feedback on whether the solution is optimal or not.
Praise good work - see something you like in a pull request? Say so! Giving a virtual pat on the back for a job well done is great for morale.
In summary, by addressing code reviews promptly we allow the team to maintain its velocity whilst ensuring that all work has more than one pair of eyes on it. Both this and the demand for adequate testing helps to ensure the long-term maintainability and quality of the codebase.
The Responsibilities of Code Reviewees
You're about to send some code for review. What should you keep in mind?
Keep your pull requests as small as possible - this is the most important responsibility when sending code for review. It is well documented that context switching is a problem for developer productivity (which is why it is often used as an excuse for not reviewing PRs in a timely manner). Mitigate this by sending small PRs so that your reviewer(s) won't have to spend a long time away from their own work. This can be done by breaking features down in to smaller tasks, rather than sending a single pull request for the entire feature.
Write a detailed yet concise overview of your changes - the pull request overview is an often overlooked part of the code review process, but is in fact a crucial element of it. The overview is your opportunity to showcase the work you have done, and also to transfer as much information about the problem you are solving from your head in to the head of the reviewer(s). Perhaps you tried multiple approaches to solving the problem before settling on the one you have sent for review. In this case, write about this process and explain your choices. This will save time as reviewers won't have to ask the question and wait for you to respond. The overview is also a chance for you to show off your work. Built a really nice animation? Add a gif so that reviewers can appreciate it!
Add helpful commit messages - a well crafted commit message adds extra context to the changes you have made. Sometimes it is easier for reviewers to review each commit separately, in which case a poor commit message can lead to confusion rather than clarity. A clear and succinct commit message, by contrast, makes the reviewer's job easier as it aids their comprehension of the code they are reviewing.
Respond to comments immediately - if a reviewer has been gracious enough to drop what they are doing to review your changes, the least you can do is show the same courtesy and respond straight away to any questions, comments, or requests for changes that they make. We should always try to maintain velocity throughout the code review process, and focus on getting work completed rather than having it stuck in a pull request for days on end.
In summary, by keeping our pull requests compact, we allow a reviewer to gain a complete understanding of the changes in a relatively short period of time, keeping team velocity high during the code review process. By writing helpful and clear pull request overviews and commit messages, we aid reviewer comprehension. This again leads to quicker reviews, but also facilitates a better understanding of the changes on the behalf of the reviewer, which makes a more in-depth review of the code far easier. This maximises the likelihood of the optimal solution being discussed and implemented, which in turn leads to higher code quality.
Practical PR Examples
Now let's have a look at what these principles look like in practice. First, an example of a poorly put together pull request:
Where to begin with this monstrosity?! First of all, the PR is way too big. No-one is going to be able to effectively review 1,491 lines of code in their web browser. Unless the majority of these changes are to asset files, and the code changes themselves are a relatively small part of the PR, it should be rejected on the basis of the size alone.
This brings us nicely on to the next problem. If indeed only a minority of the changes in this PR are to the code, then they should be in a discrete commit so that they can be reviewed in isolation. But this PR only has one commit, so if the reviewer did want to be able to focus solely on the code changes they would have no ability to do so. This is shameful stuff.
But it gets worse. How is a reviewer supposed to understand the context of these changes? The PR title is nonsense, the branch name completely meaningless, and don't get me started on the commit message. As if that wasn't enough, the overview is a single sentence that is so vague that it somehow manages to be worse than nothing at all.
Now, after that ghastly business, shall we have a look at how it should be done?
All of the problems with the previous example have been rectified. First of all, this PR is a manageable size, allowing it to be reviewed far more thoroughly as it can be comprehended in a much shorter space of time. The pull request overview is also significantly more detailed (in that it actually contains some detail), giving reviewers the context they need to understand what the code that they are about to review has been written to achieve. The screenshots in particular are extremely helpful when reviewing code that deals with UI. There is also a relevant branch name that contains the ticket number that the branch corresponds to (which is handily linked to in the overview description). The icing on the cake is that the overview also contains links to individual commits within the PR, providing extra context to them and making it simple for a reviewer to review the changes on a per-commit basis should they wish to.
While we're on the subject of commits, let's have a quick look at them:
Each commit contains the ticket number and a brief description of the changes. This again adds context to the changes themselves, making them easier to understand when reviewed. They are also incredibly useful when using git blame to figure out why a particular piece of code was written.
Conclusion
By following the practices detailed in this article, the process of code review can be harnessed to deliver high quality, maintainable software systems, whilst also preserving a high-velocity development environment. A better quality codebase paired with a robust test suite empowers engineering teams to produce new features at a consistent pace without having to sacrifice on quality. Why not try them out on your own team? You may be surprised at just how effective they can be.