nit: Fix your team’s code review participation issues
We, developers, love to write code and always spend our energy on the design and cleanliness of our code before we send it to our team for a code review. It feels creative to solve a challenging problem, come up with new ideas and implement them. But do we get the same satisfaction from reviewing other developer’s code?
Ultimately reviewing code is part of our job as much as writing it. I believe code reviews are the place where “your code” becomes the team’s code. Everyone who writes and reviews code are equally responsible for making sure that it works and it works in an efficient way.
I suppose that most companies that develop software require code reviews as a policy to protect their branches and use pull requests to easily review them. However, setting a policy that prevents developers from pushing code to the main branch without a pull request might not be enough to make sure that your team is actually reviewing code.
I have seen many teams throughout my career that had good engineering practices in place, however when it came to code reviews either the team slacked from time to time or sometimes reviewers would take a quick look, leave a few nit-pick comments and approve the pull request (I did this myself too when I felt lazy). And from the outside, no code gets pushed to production without multiple devs reviewing it thoroughly, right?
I believe this issue is one of the human aspects of software development. Tools help us do our daily tasks more efficiently way by automating processes or creating a better user experience. However, sometimes they’re simply not enough to change our behaviours. So how can we make sure that developers pay the same amount of attention to reviewing code as much as writing it and encourage more people to participate in code reviews?
Let’s understand why this happens first. I always found reading other people’s code more difficult than writing my own and I usually allocate the best time during the day to write code so that I can do it without getting distracted and giving a break for a few hours. After these sessions, I feel productive and accomplished and it’s time to get a cup of coffee and look into something easier to take a mental break. And what do I do? Code reviews. I’m obviously conflicting with myself, if I think that reading code is more challenging than writing it and believe that they’re equally important why do I pick the less productive time to do the more challenging thing?
To address the dilemma above, I believe the solution is to make code reviews easier and more appealing to developers. Here are some suggestions:
Small pull requests:
If you search best practices for code reviews you’ll see that almost everyone suggests sending small pull requests. I believe big PRs are discouraging for the reviewers because they’re time consuming and mentally exhausting to understand. As a result, the reviewer might end up delaying the PR which results in both the author and the reviewer getting frustrated.
So how can we measure the size/scope of a pull request? Is it proportional to number of files or lines changed? Imagine a PR with 100 files and each file containing the same change of renaming a class. Is this PR difficult to review? Not at all. Let’s take another PR containing 1 file with 3 lines of code change and each line associating to a different bug fix. In this case, the reviewer needs to find the mapping between the bugs and the changes. It would have been much easier if each fix was sent in a separate PR. So the size of a PR cannot be measured by number of files or lines changed, it’s more related to the context.
Why small PRs are better?
- Better for continuous integration because it encourages developers to commit to mainline branch everyday
- Happier product managers due to independent changes deployed faster
- Encourages developers to do reviews because small PRs are much easier to read
- Easier to catch bugs in a small PR and better catch them way early in the development pipeline
- Easier merge conflicts to resolve
- Smaller changes are easier to find and revert if you break mainline branch or production
- The team can intervene in the design of a big feature and decide to change it before the whole feature is implemented and it’s too late
- Juniors and newcomers can learn more from PRs if the changes are easier to understand
Don’t send a PR unless it’s ready:
So far we tried to make the review process less mentally exhausting and time consuming. This following rule is probably the most important one when it comes to respecting the reviewer’s time. So if you’re an author, make sure that the PR you’re sending is complete and ready to be merged to your destination branch. Something I do very often is I review my own code as if it is written by someone else to make sure it is ready for others to review. I find this very useful for catching bugs and cleaning up the parts I used during debugging. And remember, if your team is curious about your progress on your unfinished work you can always create a draft PR.
Automate as much as you can in the review process:
To make things easier for the reviewer automate all PR verification processes that can be automated. Such as building code, running tests and checking code coverage results. This way reviewer doesn’t have to think about these each time there’s a new PR.
Use linters and style guides to auto-fix and catch styling issues and coding convention breaks. Most repository management systems have branch policies and built-in tasks to protect your branches. And since you would be running many verification processes, make sure to run independent ones in parallel so that the authors do not get frustrated by waiting on their PRs to get verified.
Create a written checklist for what to look for during code reviews:
I suggested automating verification processes required for PRs above. However, not everything can be automated such as evaluating the readability of the code and measuring how well the new code integrates with the rest of the system. Creating a written checklist for things to look for during code reviews is a great way of depersonalizing the standards that need to be matched when writing code.
Sometimes conflicts and disagreements can occur between the author and the reviewers. Having a written list and linking the items on the list in PR comments can help people understand that these standards are not personal but they’re things the whole team agreed on. This could prevent people from taking comments personal and thus avoid conflicts.
Here’re some great checklists to get inspired:
Use asynchronous communication to avoid distractions:
If developers have to ping each other to ask for a review (even in person in some cases) every time they create/update a PR, there will be too many distractions. To avoid this situation, use asynchronous communication techniques such as send automated notifications to reviewers when a PR is created/updated but do not expect an immediate response. Set up a time window for reviewing changes such as the next business day of the update.
Allocate time for code reviews when you’re doing capacity planning:
If there’s a certain pressure on developers to finish their features, they could be rushed to do code reviews or sometimes even skip them. So if you’re doing capacity planning for the team make sure to allocate time for code reviews as well. You might think that not doing reviews could save time and accelerate feature development. However in the long run, not reviewing code properly will bring many complications to the team and the clients.
Emphasise the importance of code reviews in your 1–1s:
If you are a team lead or someone who does regular 1–1s, encourage developers to be more proactive in code reviews. Help them understand that reviewing code is as important as writing it because it is a great learning tool and is the universally accepted engineering practice that helps teams ship healthy code.
If a bug surfaces in production do not blame the person who originally wrote the code or do not let them take the blame. Always remember their code was reviewed by other people as well, they’re all equally responsible. So sometimes ask the reviewers to fix the bug instead of the author to help them feel more responsible.
Code reviews are not just for finding issues, they’re also a great way to improve yourself. If you are a junior developer or someone who is working on a new code base, asking questions on code reviews and observing different types of coding styles and design patterns can help you improve and ramp-up faster. Personally, the best mentors I had always reviewed my code in detail. So, if you’re a team lead encourage mentorship culture and ask mentors to do detailed code reviews of their mentees.
Practice reading code:
If you’re a person who does code reviews, definitely practice your code reading skills. A way to do this is by going to GitHub and finding your favourite open-source libraries/tools and just take a look at how they’re written.
Also, keep yourself updated on new tools that can help you review code faster such as Sourcegraph’s browser extension for navigating code in GitHub pull requests.
Avoid ownership areas in the codebase:
Assigning owners to parts of the code and requiring them to review the changes in their areas might sound like a good solution for guarding the code base against certain issues. You might think that creating “experts” can increase velocity of the team and make project management easier. However, usually the owners end up the only ones who change or maintain their assigned pieces of code, and when they’re not around to fix an issue the rest of the team is likely to suffer.
I personally think that ownership creates territories and territories can cause conflicts within a team. Assigning areas to developers can make project management easier however it makes people management more difficult. Instead, we should redefine territories and create a sense of ownership in the team/company. By doing this, we can avoid potential conflicts that can occur within a team during code reviews and make sure that the whole team is available to fix an issue or review changes at any time.
Avoid large feature branches:
Some teams prefer large feature branches that are usually merged to mainline branch after working on them for several weeks or months. This could bring potential problems in the review process. First of all, the merge request to the mainline branch is going to be a huge PR, very difficult to review for people who are seeing the changes for the first time. Secondly, the owners of the feature branch are likely to resolve many merge conflicts within the time frame the feature branch is open. Thus, asking the reviewers not only to look into the new code but also check if no mistakes were made when resolving merge conflicts.
I would suggest using continuous integration which I believe is a more modern approach than using large feature branches in most cases. The principal of CI is, encouraging developers to commit to the mainline branch everyday where each commit is a deliverable piece of code. Here’s a great comparison of feature branching vs continuous integration by Martin Fowler.
Be nice in code reviews:
Most importantly, be nice as a reviewer and author during code reviews :) If you feel like there’s a potential conflict coming, try to talk to your teammates in person (or video call if you’re working remotely) to help them understand that your intentions are good!
Thanks for reading!
