A nice guide on code reviews (unfortunately no author is attributed on the Notion doc) is making the rounds. If you do code reviews, you should read it. If you don’t, you should start, and then read it.
I do have a personal quibble with this particular guide. I find code review most valuable for “education and context sharing”. I rarely detect bugs or “safety” issues when reading PRs. I’m trying to build this skill, so maybe check back with me later on that.
On the flip side, I want to boost a couple ideas that I wish had been in force for code reviews I’ve given/received in the past:
- “No stylistic preferences” enforced by humans. This is indeed what linters and style guides are for. If you object to the use of linters, I can’t help you.
- “No blocking tips”. Lately I’ve tried leaving comments that start with “not a blocker…” followed by an idea on how to make code clearer. This gives the code author the option to address it immediately if they have the gumption, return to it later if they don’t, or take it under consideration for the future.
- “Any PR that introduces…a security or privacy issue should be blocked”, in particular user privacy/safety issues. I’m lucky to work on a team that is considering user safety and data privacy very early in the process, so we often don’t come across these issues as late as code review. That said, expanding code review scrutiny to include ways that customer data could be leaked or people could act maliciously feels like a cultural win.
Lastly, this whole paragraph hits home. Onboarding and “mind-melding” is possibly the second most important thing to keep in mind regarding code review in the year 2020:
Nobody went to school for hacking on your company’s stack. Outside of software fundamentals all of us had to learn how to make things work while on the job. Code reviews are one of the best ways for us to share knowledge and context about different ways things are done or tricks we’ve figured out to get things done in better ways.
In my opinion, the most important thing to remember about code review is that it is often the worst time to offer non-trivial feedback. Most pull/change requests land when a project is wrapping up and the author is ready to ship the thing. This is the riskiest and most frustrating time to make substantial changes. If you want to improve code design and practice, pair programming is the better tool, not code review.
