At some point in a Product Engineering team you will have to address some issues related to code reviews. In some teams, code is pushed without barely looking at because reviews are too light. In others, code stays there for too long because reviews are too harsh. In others, pull requests are completely abandoned because nobody actually reviews code in their day to day.
Some more mature teams like Netlify arrive at creative solutions like adopting a metaphor to give feedback on code reviews. Others, like Squarespace, create more extended guidelines to submit and review pull requests. Google as even a more extensive guide. It doesn’t have to be so pro. It can be something simpler. I want to share my approach for the teams I worked with. We actually have this written down in a Confluence page.
As a rule of thumb, a pull request must be reviewed by at least one team member from another team. It doesn’t avoid knowledge silos, but it helps to have an idea of what is happening in other teams. For that, we need to have some common rules about submitting and reviewing PRs, so the experience is as uniform as possible.
The responsibility of submitters is to make the code as easy to understand as possible. To make the reviewer’s job easier, the submitter should:
Give enough context
Use a pull request template and answer the following questions:
- What does this PR do?
- Why do we want to do that?
- What high level changes did you make to the code?
- What other information should the reviewer be aware of?
Provide information like UI screenshots or GIFs, link to the JIRA ticket, etc.
Make the PR a manageable size
More than 300 changes is a red flag. More than 10 files is a red flag. There are exceptions like renaming a package or moving files around.
Do not submit any code that is not used on the same PR
Unless the code is intended to be a public API consumed by others. This is controversial, but if the code we push to production is not used, how can we ensure we are adding value?
The responsibility of reviewers is to ensure they accept the code as theirs and will take care of it as if it was written by themselves. Different software engineers will look for different aspects of the code, but to ensure a fluid experience for everyone the reviewer should:
Buy a ticket for at least the 4 buses of code review
First thing in the morning, before lunch, after lunch and last thing in the afternoon. Having at least four times in the day where code is reviewed, helps quite a lot. It ensures PRs don’t stay too long without anyone looking at them. It also avoids personal Slack review reminders. You can use Trailer to accelerate your GitHub workflow but it’s not mandatory.
Ensure the PR fulfills the merge-able checklist
Don’t be too light on the code review. Remember that you are accepting the code as your own. Also don’t be too harsh. Different software engineers have different ways of solving a problem. For a code to be accepted, it only has to fulfill that:
- It achieves the given task
- It has been tested and the tests are passing
- It follows the agreed architectural guidelines
- It has a manageable size
- There is no unused, dead or duplicated code
- The system is not going to be harmed
- It doesn’t introduce unnecessary complexity
Do a follow up
Return to the PR if there are requested changes and propose alternatives to solve them. Do a pair programming session if there are too many requested changes.
Adapt it to what your team actually needs. Address the issues your team is struggling with and write something down. If you want to go pro, do it. Write more extensive guidelines or automate even further. But you don’t need to go that far to start improving code reviews.
Whether or not you should be even doing pull requests is a topic for another post.
Interested in working with us? We’re always looking for people passionate about technology to join our crew! You can check out our openings here.