I started a new job recently and my on-boarding mentor told me I was bringing lots of value to pull request reviews despite being the new guy. This caused me to pause and reflect on how I approach code reviews, what I might be doing differently from others, why I do it and where my habits came from.

Perhaps the best way to describe my approach is that I think of code reviews as an asynchronous pair coding session:

  • If you pay attention during pair-coding (and you should), pay attention while reviewing PRs.
  • If you’re engaged during pair-coding (and you should be), be engaged while reviewing.
  • If you try to be helpful while pair-coding (and you should), help your peer out while reviewing.
  • If you question assumptions while pair-coding (you should be, kindly), do so while reviewing.
  • If you trust but verify (and you should be, respectfully), do so while reviewing.

However, it’s possible that productive, helpful, informative pair-coding sessions are a rarity, so let’s break things down a bit more. Maybe your pair-coding experience and tendencies are different than mine.

Ask questions

  1. Understand the code being changed
  2. Understand the context, the surrounding and the calling code, at least at a high level
  3. Understand the problem that needs solving, and why
  4. Does this change solve the problem?

Ask questions if you don’t understand.

Don’t just go along for the ride

  1. Think through the same problems your peer had to think through. Question assumptions and check documentation.
  2. Pay special attention to logic and algorithms so you can spot holes or edge cases that should be handled.
  3. If your company tries to follow a style guide, ensure the code does.
  4. Consider how future you, or a future peer, might think about the change being made. What might be done to improve clarity, and reduce mistakes in the future?

If you’re not doing all of the above, you’re wasting everyone’s time. That seems a bit harsh, but it’s true. You’re squandering the opportunity to catch a bug that might otherwise make it into production. You’re squandering the opportunity to catch logic errors or surface that gap in edge-case handling. You’re squandering an opportunity to learn, which would make yourself more productive and valuable. Don’t rubber stamp simply because your peer needs one. Your peer shouldn’t get insulted if you “trust AND verify”, AND act as a second-brain, AND help improve the quality of their work, AND fully understand what you’re signing off on.

Thanks

I’d like to thank my peers at DeviantArt for ensuring diligence during code reviews. Specifically banks, shadowhand, muteor, helloandre, kemayo, inazar and of course randomduck. I worked with many people during that time, but I specifically remember those peers prodding more towards higher quality. So thank you!