Reviewing Code like a Pro

There are three key components to doing code reviews professionally.

1. Be thorough

You know what's embarrassing? Giving someone else's code an LGTM, only to have a second code reviewer point out a key bug that would cause major production issues.

Clicking "yes" on someone else's code without reading it doesn't do anyone any good. It doesn't do the author any good, because now they're not getting constructive feedback that they can use to improve. It doesn't do you any good, because you're executing an algorithm that a machine could do for no benefit. And it doesn't do your company any good, because you could be missing production bugs or just issues that make it harder for someone else to read the code.

You're an engineer being asked to do code reviews. That means you're literally being paid to complain about other people's code. Come on, that sounds at least a little bit appealing, right?

There are three questions I ask myself to verify that I'm being thorough enough with a code review:

1. Are you increasing the bus factor of this code? In other words, if the author went on parental leave for three months starting tomorrow, would you be able to maintain or answer questions about the code you've reviewed? If not, it's worth looking harder to try and understand the change that's being made.

2. Are you confident in the correctness of the change that's being made? Not just "do I trust the author to write good code," but "do I trust my own assessment that this change is good?" The goal is to do a real cross-check from your own logical intuitions.

You should also check for automated tests that would help you feel confident in the change if you were submitting it. Unit tests, integration tests, benchmarks - whatever would be appropriate given the change that's being made.

3. Are you confident that the next engineer will be able to understand and modify this code? If not, maybe it needs docstrings, a README, clearer organization, better naming, or all of the above. Asking for a docstring on a function is not a nitpick, it's critical for ensuring the code's maintainability.

2. Be appreciative

Getting negative feedback on your code kind of sucks. There's no getting around that, and engineers have to get used to taking constructive criticism. But there's no need to be an asshole about it. You can make your feedback a lot more easy to swallow by taking the time to appreciate the author's work.

• Comment on parts that you think are especially good about the code. Well-written documentation, clean organization, thorough tests. Try not to go overboard with this or it can seem condescending -- to avoid that, focus on things that you personally benefit from. "This docstring really helped me understand the code, thanks."

• If appropriate, give an overall comment on the PR as well as individual code comments. Something like "Looks good overall, I have a lot of comments/nitpicks on small issues." These comments can feel formulaic to write, but I always feel they soften the blow of a tough review when I get one on my own code.
  Plus, they're almost always true: if you really didn't think the overall idea of the code was good, why would you leave 30 comments nitpicking it? It would be a waste of time to do a detailed critique on something that you think ought to be totally thrown away.
• And if you do think their code needs to be thrown away, well, be a human being about it. Address the idea and the consequences, not the person. "Hey, I think there are some serious design issues here and this needs to be reworked. Here's why and what I think we should do instead."

• Respect the author's time by letting them put off larger or less urgent comments to a later PR. I once worked with someone who was always saying "Hey, while you're in there, could you also... ?" Little bits of this can help make the codebase better ("while you're in there, can you fix this one style guide violation right next to the code you changed?"), but too much of it gets exhausting ("while you're in there, can you refactor the foobar? It's only 500 lines of code"). Don't treat it as their fault when pre-existing code is bad.
  If you have something you want to comment on and you're afraid it'll get lost in the shuffle later, you can say "Doesn't have to be in this PR, but I think we should fix X so that it stops doing Y." You can ask the author to file a ticket to follow up, or even file the ticket yourself. Don't use them as your code-cleanup slave just because they need your LGTM on the code review. 

3. Be fast

Slowing down your code review slows down the productivity of the author by way more than 1x. It blocks them if they don't have something else to do, and imposes switching costs if they do. For that reason, I think everyone should prioritize requested code reviews above writing their own code.

Some tips for reviewing code faster, while staying thorough:

• Don't try to task-switch while reviewing code. Focus entirely on the review. You'll get it done both faster and better. Take notes if you need to to understand it more thoroughly, or ask the author.
• Think through your comments... but don't think about them too much.
  
  Example 1: If you see something weird in the code and comment "why did you do this?" and then read on and see why they did it, you can probably delete your comment.
  
  Example 2: If you see something weird where you think it might affect the performance, but you're not sure, spend at most 5-10 minutes looking it up before just shooting off the comment and asking the author to investigate further.
  
  There's a cultural expectation of balance between how much work the reviewer vs the author is expected to do. My sense is that this works best if most of the work is presumed to fall on the author, because it's better incentive-wise; they are the one who wants to get their code merged.
  
• Approve the code as soon as you are sure the author will address the comments to your satisfaction.
  
  Things that should not be approved: Code that needs major changes. Code whose author may not understand how to respond to your comments appropriately.
  
  
  Things that can be approved: Code that needs minor changes, when you're confident the author will agree and fix the changes as you expect. Typos, style guide fixes, even small logic changes if you're pretty sure the fix is uncontroversial.
  
  
  Don't approve too early or the author might feel obligated to ask you again if you really mean to approve the code in its current/fixed state. I usually approve when the code is down to a few minor changes.

Meta-level ways to review code faster:

• When you get a review, drop the other things you're doing and do it first.
• Prioritize code review and communicate up that you're doing that, and that you consider it important.
• Show appreciation for others in your organization who do fast and quality code reviews, e.g. mention to your manager that X's fast code reviews are improving your productivity.

A culture of fast, kind, and detailed code reviews can improve a team's output dramatically. It's worth prioritizing.