On Code Reviews
Code reviews are a really important part of the engineering process. However, they can become onerous, especially with certain attitudes and dispositions by code reviewers. If approached with the wrong attitude, code review can be enervating, demoralizing, unnecessarily slow down everything sometimes even become downright toxic and destructive. It’s critical to approach the process in a way that maximizes value for everyone involved and minimizes cost and effort. Your job as a reviewer is to unleash the productivity of your teammates while maintaining quality standards, readability, and the intellectual cohesion of the project. It’s an essential skill for working within software projects. This article describes approaches and attitudes to code review that have proved useful and effective.
Reviewing Code is Not Writing Code
If there is one thing you remember from this guide it is this: the goal of a code review is not to make it so that the code looks as if you wrote it. The goal is to ensure that you agree on overall direction, to ensure that certain quality bar levels are hit, and — maybe most importantly — to serve as a vector of communication from the code writer to the code reviewer (and the rest of the team) about what is going on. Do not attempt coerce the code writer to change the code to look like it’s yours. Suggestions are fine; common style guidelines are fine; but sometimes you’ve got to show some level of flexibility if it wasn’t written exactly the same way you would have written it.
Delegate to Tooling Whenever Possible
With modern code tooling such as linters and code formatters (e.g. yapf in python and prettier in the JS ecosystem) combined with CI/CD (Travis, CircleCI, or similar) there should be precisely zero time spent reviewing formatting errors and catching similar programming and styling errors that can be detected by a machine. A reviewer probably shouldn’t even note them; just let the automated systems catch them. Computers are infinitely better compilers, linters, and code formatters than humans are. Let the computers do these things.
This is not just an efficiency win, but a cultural win. Formatting wars in code bases can be a source of significant stress and tension. Style guides can get out of date, and code reviewers can be inconsistent in their levels of rigor or pedantry, depending on your point of view. Instead let tooling smooth out the interactions between humans and prevent squabbles before they ever begin.
Consistent Granularity
Reviews and commentary should be done on the same level of granularity. From a code author’s perspective, it can be very frustrating to request feedback on the overall direction of a PR or a system, and have the review focus on details that do not matter at the moment. If you go through a PR and annotate a bunch of tactical changes and then at the end request changes and write something like “Actually I think you should rethink this entire approach”, that is a fail on the reviewer’s part. It’s fine to rethink approaches, just don’t bombard the PR with tactical feedback that will no longer be relevant. An author might fix tactical changes as they go along only to find the bombshell that they have to start from square one.
A Nit is Not Enough
If your modifications and suggestions are nits and minor style issues, that is not a sufficient bar to request changes. Caring about nits is important. Pride in collective work and making things pretty is important. You should care. But you shouldn’t allow it to unnecessarily slow things down either. Make your comments, note the nits, and just accept the PR for heaven’s sake. Trust that your colleagues are respectful and that they will make the changes you wanted before merging or have a reasonable reason why they did not. If they frequently do not, that is their bad and you need to talk to them or even use formalized feedback channels if necessary. If you don’t trust your colleagues, well then you and your team better work on that.
Stop Typing and Have a Conversation
Avoid long-winded discussions on PR whenever possible. Avoid the temptation to do a knock down drag out fight on a PR. If you are not reaching convergence quickly with a reviewer, don’t spend 20 minutes crafting your next response. Instead just take five minutes and go talk to them or set up a call if they are working at another location. It almost always works better. For posterity, always note what was discussed/agreed offline back to the PR.
It is worth noting that long PR discussions are sometimes appropriate on system-wide changes with a lot of stakeholders as it serves as a useful written record of the discussion for all to seem. But that discussion should be an explicit goal of that PR.
Build Trust
Frequently the root cause of code review thrash is the issue of trust. If a lack of trust is involved, it does not matter how good the code is. This is especially true when author is changing code that someone else feels ownership over.
Trust is not built at once. It takes time. Nothing builds trust better than being a good engineer to work with: Write high quality code. Write manageable PRs. Write good PR summaries. Write good test plans. Actually follow your test plans. If you do this consistently, people will trust you, and they will both accept PRs faster and more readily listen to your feedback. You will move faster.
Request Changes is Not Personal
Learn to embrace “Request Changes” in a PR workflow. Don’t take it personally. It should be interpreted as “Hey, I have some feedback so popping this back to you so we can discuss.”
Within the most effective teams I worked within while at Facebook, we used PR (we called them diffs) workflow as a sort of task list. You could open your PR management tool, and see all the action items related to PRs in the form of “Waiting on You” — a PR that needed review — and “Changes Requested” — a PR where you need to address feedback. I even used this to bounce back PRs where I was confused and didn’t understand enough to review it appropriately. (The other action in that case was simply to “Resign”, which also should not be taken personally).
You are not your code. When someone gives you respectful feedback on it, detachment is the appropriate state of mind. Embrace “Request Changes” and use it to tighten your feedback loop and increase your velocity.
Find people to emulate
The best way to learn how to review code effectively is to find people who do a good job and emulate and learn from them. It’s a matter of taste that has to be developed over time. The best reviewers demand excellence respectfully, consistently focus on the most salient and important parts of a diff, and never unnecessarily slow you down. That is what you should shoot for.
