Fortunately, I don’t think the necessity of code review is a controversial topic these days for most tech companies1. However, it is not always implemented as well as it should. In this post, I will describe some practices that I try to follow when opening pull requests, and that I’ve been encouraging co-workers to adapt in order to maximize the benefits of code review.
Let’s start by defining some goals of the code review process:
- Sharing knowledge of a new feature/fix. When done properly, the reviewer will have context about what got merged. This will be extremely useful down the road when she has to adapt the modified files.
- Minimizing mistakes and bugs. Ideally, they are caught by tests, but we know that’s not always 100% possible. An easy example: you might have forgotten to add the test that would have failed.
- Asking for feedback about different alternatives.
- Sharing new techniques/designs.
In my experience, if pull requests are not easily reviewable, reviewers will end up procrastinating, and ultimately giving the classic and reckless “lgtm, 🚢 it”. This is terrible, and it is actually worse than not reviewing code at all (it leads to a false sense of safety). Avoiding this should be a priority for every company with a strong engineering culture2. Reviewing code should be an exciting opportunity to learn from other people, get more context about the codebase, and ultimately, improve the product that is being built.
These are some simple practices that I like and can be easily implemented by teams of any size and experience level. These can drastically change the effectiveness of code review, and make the life of the reviewers much, much easier.
0. Proper title and description
This is obvious, but we all forget sometimes. Make the title clear, and give enough context in the description. Some examples:
- Is it a response to a customer request? a bug fix? a new feature? Link to the appropriate ticket.
- Are you modifying the data model? Describe the new tables, columns, or relationships briefly.
- Are there new endpoints/public API methods? List and briefly describe them.
This will totally change the mindset of the reviewer. When she starts looking at the files, she will already have an idea in their head about what has to change and will be prepared to provide alternatives or optimizations. An easy trick is to use pull request templates.
Before you mark a pull request as final or ready for review, give it a self-review. You will be surprised how many things you will catch by reviewing your own code after you have pushed it. From variable names that are not meaningful or inconsistent style to feedback that you have received in the past. It’s fascinating how many times we break the rules in our own code, that we later try to enforce when reviewing other people’s code.
Sometimes, there might be a lot of autogenerated code or the order of the files might be confusing. Leaving a clarifying note will make a big difference.
2. Highlight the most controversial/critical changes
As a continuation of #1, comment on your own code! Particularly the most critical or controversial changes, or changes you are not 100% certain about. This will help to catch the reviewer’s attention and minimize the chances of quickly dismissing the change. I’ve seen many mistakes caught this way or smart optimizations that came through the collaboration of multiple team members.
3. Ask for feedback earlier
Particularly if the functionality is large, don’t wait until the day before the deadline to ask for a review. Sometimes, early feedback can catch design problems or systematic flaws.
4. Follow the style guide
It does not matter what convention your team follows. But follow one. It is super distracting to review code that has a clearly different style. Try to automate the process as much as possible (with linters and code formatters), but also have clean conventions for the things that cannot be automated. It is not that important when the team is small, but it will pay dividends when the team grows.
5. Proper sizing
This one is easier said than done, and it’s an art. But size your pull requests in a way that feels good for the reviewer. It should not be too big or too small. If it’s too big, the review will not be that exhaustive. If it’s too small and references many other pull requests, reviewers might forget how they interact together, and potentially miss issues.
6. Automate everything
Try to minimize the honest mistakes that are common in your codebase by testing for them automatically. Take advantage of static analysis tools to avoid patterns or functionalities that are not encouraged in your team.
Remember that we end up spending more time reading code than writing it. Following these guidelines can drastically change the readability of the pull requests in your organization, and ultimately the overall readability of the codebase. These steps should make the reviewing experience a little bit more pleasant and effective. Do you have any other tips to improve the review process? Please reach out!
1If code is not reviewed in your organization, please fight to introduce the practice. If you are not successful, RUN.
2Note that these mistakes are not only made by junior developers. In fact, junior developers typically do better. They tend to ask more questions and peers try to provide valuable feedback. When a pull request is opened by a senior member, sometimes people are afraid to ask questions or provide feedback. And generally, we overestimate senior engineer’s attention to detail, when in reality, nobody is free from making mistakes.
Subscribe via RSS