Code reviews are one of the best tools to improve quality of software without requiring too much effort compared to other processes including audits, training and maintenance. Yet, it is simple to fall into the trap of thinking that only a few people can perform code reviews. Here is our first advice: everyone can (and should) do code reviews. We have worked with people from different backgrounds and code reviews became our silver bullet to involve them. Usually, people become more engaged and efficient when you trust them.
Formal vs lightweight code reviews
You may also wonder when and how to review code. Should you do code reviews at the beginning of the project? At the end? How many times during the development phase? Should it be formal? There is not a single answer to these questions but we recommend to start reviewing the code as early as possible, and to schedule many lightweight code reviews thereafter. Code reviews do not have to be formal to be very effective. You do not need to book a meeting-room and invite the whole team: a code review is not an “exam”. It should rather be seen as a best practice like writing tests or updating the documentation. Hence, code reviews must not be sanctified. Just do it™.
Yet, it can be costly to perform a code review, especially when you do not rely on the right tools or when you do not have any or too few guidelines. Repository managers such as GitHub and GitLab significantly helps here, thanks to the concept of Pull/Merge Requests (PRs/MRs), i.e. proposing code changes. By adopting PRs/MRs, you actually adopt a (git) “flow”, which favors code reviews. Pull/Merge Requests are created on GitHub/GitLab, and visualizing what has changed and leaving comments on any line could not be easier.
At TailorDev, we have defined a set of recommendations to perform code reviews in an efficient manner. The next sections will describe different points anyone doing a code review should be aware of and respect.
1. Ask. Do not tell.
Always ask for clarifications when you do not understand a line or a design decision. Do not be too direct by making demands. Instead, ask:
I would not have done this that way, what about using X from lib instead?
Not sure to understand. What do you want to achieve here?
2. Be nice, positive, factual, helpful
Do not undermine the author(s) of the PR/MR. Be constructive and factual to avoid misinterpretations and/or personal attacks. Most of the time, you are part of the same team, or at least of the same company, which means both of you are in the same boat.
Be as nice as possible, especially when working remotely. It is very complicated to convey emotions, tones and intents within a web page full of comments. Use Emoji for that. Additionally, it would be wise to avoid jokes and sarcasm for the exact same reasons.
Being positive and helpful is very important too. Suggest alternatives by asking questions (cf. First recommendation). Be explicit to avoid communication issues. Reviewing code is not an easy task, but so do is to collaborate with other humans. Using a tool between is not an excuse to forget that you are working with other people. Never write something you would not say in front of that people.
3. What is under review was the best option at the time of writing
Never assume the authors did not do their best to provide the work you are reviewing. They are intelligent and good people. They used/relied on everything that was available to us when they worked on this Pull Request. That is also why the two previous points are essential.
Now, your job is to engage a discussion with the authors to reach a consensus between your expectations (and opinions) and what you are reviewing now. It is always a matter of trade-offs, but be sure not to waste time.
4. Focus on the important parts, not on the details
You may be familiar with the citation below, which we find very accurate. Let’s be honest, reviewing hundreds of changes is very complicated and I’ll get back to that in Section 6.
Ask a programmer to review 10 lines of code, he'll find 10 issues. Ask him to do 500 lines and he'll say it looks good.— Giray Özil (@girayozil) February 27, 2013
What is really important here is to ignore all the cosmetics changes you would make. This does not mean coding standards are not important, but this is not something you want to discuss during a code review. I will get back to that in Section 7 too.
Once you understand the purpose of the changes and why it is necessary, be sure that the code actually works and is covered by tests. Think about (realistic) edge cases and ways to simplify the code. If you start commenting every line of code, stop, step back and get in touch with the authors by Slack or Appear.in for a live code review session. Then, leave only one comment with the minute of this discussion.
You may have to review code that do not fit your minimum quality threshold at all. Stay focused on the algorithms and the code actually solving the problems. Open new issues to refactor the rest later. You can use these issues to teach the authors about a specific topic (e.g., exceptions handling in Java). We do not mean to accept everything all the time, but you may have forgotten the joy and satisfaction of seeing her work merged
5. Do it often
At the beginning of this article, we introduced this concept of “lightweight” code review. The more you do, the better it is. Use a Git flow and stick to it. Each code change should imply a Pull Request and someone has to review it.
You should not wait for the complete feature to be pushed in a branch to review code. We advice to open pull requests as early as possible so that anyone can start reviewing code before it gets too big or it is too late to change things.
6. Review small changes
Reviewing small changes allows to avoid the trap mentioned in Section 4. As a general rule of thumb, we recommend around 300 changes for a single Pull Request. It may vary, for instance depending on the programming language, but this figure has worked well for us so far.
In any case you should spend more than 25 minutes on a code review. If it takes more time, break it in multiple sessions. GitHub has introduced a feature where you can start a code review and send all the comments at once. That is particularly useful in such a situation.
7. Automate trivial checks
In order to focus on the essential, you should avoid to comment on coding standards, style, and all other things a tool could do on your behalf. No matter which programming language you use, google for “static analysis $LANGUAGE”, and you should find something like Sonar, Eslint, CPD, PHP CS Fixer, etc.
This is quite important to stay focused and avoid too many comments too. These tools are good for your project and your team, you should use them!
A note for the authors
In this article, we use the term “authors” to refer to the people who created the changes provided in a Pull/Merge Request. You should not be afraid of getting your code reviewed. If the reviewers follow the previous recommendations, they will not make you feel bad!
In this case, be grateful and take each comment as a way to improve both your work and yourself. Reply to each comment with as much context and details as you can provide. Then, address all the requested changes in different commits and ping the reviewers for another round.
Thereafter, one may ask you to squash your commits. This is useful to avoid merging commits with message like “Fix blablabla” or “Fix again”. Last but not least, either the reviewer will merge the Pull Request herself or you are free to do it, if and only if you get a.k.a. Pouce Driven Development (Thumb Driven Development). GitLab has a great “approval” feature by the way.
We are open source contributors and maintainers of different projects. These recommendations are the result of several years in the Open Source community, but we have also been inspired by many other companies and great people. You will find notable articles on code reviews below: