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.
Code reviews are an essential part of our 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 and sometimes even downright counterproductive. If done wrong, it can be demoralizing and unnecessarily slow down everything. It's critical to approach the process in a way that maximizes value for everyone involved and minimizes cost and effort.
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 about what is going on.
Do not attempt to coerce the code writer to change the code such that it was written by you. 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.
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 diff or a system, and have the review focus on details that do not matter at the moment. If you go through a diff 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 diff 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 our 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 accept the diff, for heaven's sake. Trust that your colleagues are respectful and that they will make the changes you wanted before commit 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 our formalized feedback channels if necessary. If you don’t trust your colleagues, well then you and your team better work on that.
This does not mean that you shouldn't be detail-oriented, or that you shouldn't request changes based on details. "A nit is not enough" should not be translated to "A detail is not enough." Nits are non-controversial style issues that should probably be described in a document or be well-known as a convention within a team.
Notice! Be aware of the new Needs Final Review process
If you approve a diff with comments or nits, be aware that changes after approval will trigger the need for a "final review". In most cases this is not land-blocking for the author but will need to be completed in the next 10 days. In a few sensitive code paths the diff will actually require another approval after any changes before landing.
For more details see: Phabricator/CodeAuditing/Needs Final Review
Stop Typing and Have a Conversation
Avoid long-winded discussions on diffs whenever possible. Avoid the temptation to do a knock down drag out fight on a diff. 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 VC if they are working at another site. It almost always works better.
It is worth noting that long diff discussions are sometimes appropriate on system-wide changes with a lot of stakeholders -- PreparableXHP is a good example (see D142783). However this is the exception, not the rule.
For posterity, always register what is being discussed/agreed offline back to the diff.
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. You will probably have a hard time reviewing it. This is especially true when changing code that someone else owns. You can usually assume they trust you to be a good engineer and to make sane decisions. What you cannot assume though, is that they trust you to have all the knowledge they have when making those decisions.
So, build trust. The best way to do that is to talk. When starting the discussion be ready to listen and be open to change. Make sure that you understand their values, needs, context, and goals. Make sure that they trust that you understand that and will act accordingly. If you feel that the issue at hand is controversial, go into the discussion assuming that you're wrong. Try to listen and pay more attention to what they say rather than what you think is true. Bear in mind that often the simple act of listening and understanding helps more to build trust than a pile of awesome code. It's not that you shouldn't stand your ground. You should. Defend your decisions. They are as important as the other person's are, but not more. Explain what you are doing and why, so when the diff actually arrives they will have context.
If there are trust issues within a team, leverage your manager. Talk to them about it.
Trust is not built at once. It takes a few (often offline) interactions. Nothing builds trust better than being a good engineer to work with: write manageable diffs. Write good diff summaries. Write good test plans. Actually follow your test plans. If you do this consistently, people will trust you. You will move faster.
Ask Questions
The author is usually the expert, and has probably thought about the implementation in detail. Hence, don't be afraid to be wrong or sound stupid if you don't quite understand the diff. Chances are the author has provided insufficient context and explanations to the problem he/she is trying to solve. As reviewers, we need to fully understand the diff in order to provide useful suggestions. Phrase your suggestions as questions so that the author can evaluate the pros and cons instead of blindly implementing it.
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.
Don't be afraid to put it back to the author's tray
There are reasons why the company has renamed this from "Request for Revision" to "Back to Author", one of which is to encourage reviewers to use this to get the author's attention. Context switching is hard, and authors usually want to address all the concerns at once instead of going back to the diff multiple times a day. So it helps for authors to know that the review is completed and they should check out their feedback and suggestions. At the same time, you may have questions that should be answered in order for you to continue reviewing. So notify the author of those questions by putting it back to their queue. Remember that requesting for the author's attention is not rude.
It's OK if you don't know
Identifying problems is usually easier than coming up with solutions. But don't let this stop you from highlighting why you think it's problematic in the first place. This could be as simple as “I don't like that class name but I have no ideas for a better name.” Provide reasons why the current solution is problematic, admit that you do not know of a better solution, and ask the author to think about the problem some more. Gives others a chance to see your perspective and perhaps, they can propose a better solution. You can offer to brainstorm together as well.
Final Review
This also isn't the "end," and you don't need to worry too much about whether your partner will actually "do it right," thanks to a new (well, new-ish as of August 2021) feature called "Final Review." Some truly important diffs that touch product code and have privacy implications will show up again for your "final review," allowing you to verify that the changes you requested when you approved the diff were really done. If not, you can raise a concern there and ensure that it really gets done. (Or you can just fix it yourself, or let them know -- all depending on the impact!)
Top comments (0)