In software development, the code review process stands as a crucial checkpoint for ensuring code quality, fostering collaboration, and promoting knowledge sharing among team members. Despite the importance, many engineers lack a clear mental map of how effective reviews work. This is my attempt to help code reviews and reviewers improve.
First review question: What is the intention?
Focus first on the git commit title and message. Do you understand the description? Does the proposed change make sense based on only the description? Is the idea clear? Ask yourself if you can come up with reasons why the change should not be made, or if there are obvious better alternatives.
If the description does not make sense to you, immediately share that as your initial feedback. If the description about the intent is incomprehensible, don’t waste time reviewing code implementation details. It could be that the submission was still just a draft, and the only (and immediate) feedback should be a request for the submitter clarify their intent.
The next thing to check is if the code matches the description. For example, if the change proposes to fix a bug, it should not include extra code that adds a new feature. Sometimes when you review the code, 90% of the changes make sense and do exactly what the git commit message stated, but there might be some code lines changed that you don’t understand. Ask the submitter about these. Maybe they were included by mistake, or for an unobvious reason. This should be addressed by some comments inline next to the code, or by amending the change description.
Next: Is the implementation good?
Assuming the description and changes align and make sense, shift your focus to the implementation details and assessing how the problem was solved. As a reviewer, you should ask yourself: Are there alternative ways to achieve the same outcome? Would these alternatives bring clear advantages, or is the current proposal the most sensible one?
If the code changes look mostly good and you don’t anticipate a complete overhaul in the next revision, proceed to giving feedback about the details in the code. Look for potential logical errors, deviations from coding conventions, anti-patterns, spelling mistakes, and even hint at any security concerns or performance issues you may anticipate. Scrutinize every aspect that catches your eye and suggest improvements wherever possible.
If you find yourself commenting on nearly every code line changed, consider aborting the review and asking the submitter to address the first set of comments before completing a comprehensive review.
Beware that some people might take every comment literally. When pointing out things that can be improved, remember to include phrases like “in general it is better to..” or “if you have time do..” to soften the general feedback, and clarify what actually needs to be addressed before you can give your approval for the change, and which things you merely suggest as quality improvements without insisting on them. Also beware of submitters who address comments mechanically. Instead of giving your own proposed better implementation as a code snippet, simply ask the submitter to “please rethink X so it always does Y without doing Z” which forces the submitter to write the better version themselves, and in the process train their brain muscle. Who knows, after thinking about the thing you pointed out, the submitter might even write a better fix than you initially thought of.
Nitpicking is not only OK, but actually important
Some may be hesitant to nitpick in code reviews, as nitpicking is not something civilized people tend to do in normal daily interactions. However, nitpicking is an integral part of code reviews and software engineering in general. Computers read every dot and comma literally, and these need to be exactly correct. The whole idea of a code review is to maximize quality and minimize the room for error, in both the short term and the long term. Thus, in the context of a code review, please nitpick as much as you can.
The amount of feedback should be maximal, but the bar for approval does not need to be maximally high. The requirements for minimum quality should be suitable for the skill level of the developer team. Yes, over time the quality bar can be raised as the development team collectively learns about best practices, but if the quality bar is too high from the onset, it will discourage developers from making new submissions. And the iterative cycle that allows for learning to happen won’t take place.
Managing quality is hard, but ensuring acceptable quality is central to the code review process. As a general rule, however, if you are in doubt about what is reasonable to require, err on the side of demanding higher quality rather than settling for something you feel is substandard. In the long term, you are more likely to be happy about having made such a decision.
Keep in mind that the main reason people settle for low quality is that doing things at the highest quality level requires more effort, and some people are lazy. However, if people are constantly pushed to do things at high quality, doing things correctly will soon become effortless.
Continuous integration and tests saves everybody’s time
To support the review process by humans, all software projects should have a CI in place that runs automatic tests on every code change and helps detect at least all easily detectable regressions. Code submissions with failing CI typically don’t attract many reviewers, so submitters should review and fix all CI issues themselves as soon as possible.
On a related note, all high quality code submissions that change the program’s behavior should also update and extend the associated test code. If, for example, the software project has unit tests, and a submitter sends new code without new unit tests, the reviewer should request the submitter to write the missing tests. This feedback can even be automatic – a good CI system could detect a drop in test coverage.
Communicate clearly and precisely
In the review process, it is important that both the submitter and the reviewer communicate clearly and precisely.
From the reviewer’s side, this means that the reviewer should be clear on what things are required from the submitter and what things are just nice to have. If a reviewer thinks the whole approach is bad, they should be frank and reject the submission from the get go.
From the submitter’s side, the initial submission should be as complete as possible. If code is submitted for review (e.g. Pull Request or Merge Request) but it does not have the final contents, the submitter should be explicit about this and prefix their git commits or the review title with WIP: to signify that it is work-in-progress.
In my experience, the typical reason for code submissions and reviews getting stalled is simply unclear communication. The code submitted might be fully correct, but the plain English part is lacking, leading to miscommunication. Typically, reviewers also postpone diving into submissions that they don’t understand at first glance, as having to do detective work to figure out an unclear code submission can feel overwhelming. Therefore, the best way to ensure submissions are reviewed timeously is to communicate clearly.
Avoid noise, maximize “signal”
Needless to say, both submitter and reviewer should know how to properly use the review tool. For example, both GitHub and GitLab have “Start review” and “Finish review” features that allow the reviewer to write multiple comments without spamming the submitter with multiple emails. Pressing “Finish review” will trigger the submission of one single email with all comments. Most systems also have buttons for requesting review and re-requesting review that the submitter should use to communicate clearly when the review feedback is addressed.
When a submitter re-submits code for review, they should always use git push --force. The reviewer is always looking at the submission with the question “Is this ready to be merged?” in their mind, and the submission they look at should be the final polished code. There should be no WIP commits or multiple intermediate git commits – the reviewer is not interested in how the submitter ended up with the correct end result. Only the final version matters.
Respect people’s time, prioritize correctly
Reviews require time. The submitter should be extra diligent not to waste the reviewer’s time. Likewise, the reviewer should try to review as quickly as possible, or at least share their initial impression as a quick short comment.
If reviewers are short on time, they should prioritize re-reviewing submissions they’ve already once given feedback on. People who already have context about something should continue on it, as it is most efficient for them. Other people on the team are more likely to review code submissions that have no reviews at all.
Likewise, submitters should prioritize responding to review feedback and updating and polishing their existing submissions as soon as possible. A reviewer is much more likely to re-review and approve a submission they have already looked at a couple days earlier than submissions that have been lingering for weeks or months.
Honor original code, let submitters feel ownership
A code submission and the review process serves as a process for the new submitter to become initiated with the software project. Reviewers should keep this in mind, and not rush to grab the code, but put in a little extra effort to guide the submitter on code base and on the quality bar. Reviewers might feel frustrated at times, but that is not an excuse for bad behaviour.
Sometimes, the reviewer might be tempted to fix the issues in the submission themselves instead of giving the original submitter a chance to do the final polish. A reviewer must resist this, as this will kill the feeling of ownership of the original submitter. In the context of open source projects, grabbing somebody else’s submission and committing it yourself might also constitute a copyright violation, and it certainly does not encourage the submitter to continue making further submissions. The reviewer should at most just rebase the commit, nothing more.
If the review process takes too many rounds of back-and-forth, then as a compromise the reviewer could merge the submission as-is, and immediately follow up with their own changes on top of the commit.
In both open source and company-internal work, the achievement that one’s code got merged is very valuable, in particular if it was the first accepted contribution to a particular project from that person. Don’t rob this from code submitters; let them earn it and have their git credits and purple GitHub merge badges in their profile.
There may be many reviewers, but never more than one code submitter
A fundamental principle in code submissions is maintaining a single code submitter. While there may be multiple people reviewing and posting comments, there should never be more than one person submitting the code (and subsequently improved revisions). Having one author ensures a clear owner, who iterates the submission and decides how to address all feedback. If using a git branch, that person is the only one who amends commits and force pushes the branch until the final version has been completed.
If the submission is a patch on a mailing list, having multiple submitters of a single email is impossible and authorship and ownership not an issue. On GitHub or GitLab, this problem might arise, as the submission is a git branch, and the branch permissions may allow multiple people to push to it. Having multiple people pushing to the same pull request, however, is plain wrong.
In some projects, git pull requests are intentionally misused by having multiple authors push git commits on them and discussing the collaboration in the “review comments” section. The correct way for a group of people to write code together in git before the code goes on the mainline branch is to have a feature branch. Both GitHub Pull Requests and GitLab Merge Requests allow the submitter to select the target branch of the submission. With a feature branch model, each collaborator submits their own individual pull requests towards the feature branch, and once the feature is complete, the feature owner carries the responsibility of getting the feature branch accepted into the mainline branch of the git project.
Sometimes it may happen that there is disagreement or laziness, and the submitter refuses to properly address all feedback. In these cases, the recipient/reviewer either dismisses the submission completely or accepts it as-is (merges on target branch), where work can continue in the form of follow-up submissions from other people to further improve it.
It may also happen that a submission has a great idea, but the implementation is bad and unacceptable. The submitter is asked to improve the implementation, but the submitter might fail to do so. In this situation, nobody else should rewrite that same submission, as it blurs the lines of authorship and ownership. Instead, the other person with a better implementation should simply open a new pull request or submit an email with their own version and in their own name. After that, it will be up to the reviewers to decide which submission gets accepted and which one declined.
Code reviews are opportunities to learn – for both the code submitter and the reviewer
Keep in mind that the reviewer does not need to be a superior developer. Anybody can be a reviewer, no matter how senior or junior a software developer they are. Reading code written by somebody else is a good way to get exposure to various coding styles and various ways that different developers solve coding problems. Reviewing code is a learning opportunity for the reviewer, and thus the review process ultimately leads to both the submitter and the reviewer writing better code in the future.
If you have an opportunity to do code reviews, do it, even if you don’t feel like an authority on a specific domain. You can still make a valuable contribution by giving feedback on some aspects, while leaving the final approval decision to a domain expert.
Remember to communicate clearly – the better the code is documented, and the better the feedback is explained, the more both submitter and reviewer learn from the experience.
If you have additional tips on best pratices for code reviews, please add a comment below!