Good Code Reviews

This resource first appeared in issue #13 on 06 Mar 2020 and has tags Technical Leadership: Software Development, Technical Leadership: Code Reviews

Michaela Greiler on Code Reviews - Software Engineering Radio Episode 400
How to do High-Bar Code Review Without Being a Jerk - Andrew King
How to do a Code Review - Google Engineering Practices Documentation

Reading Chelsea Troy’s blog has kind of convinced me that Code Reviews are a way of doing asynchronous, distributed pair programming. And even if you do them within an in-person team, they require good communication skills to be productive and drama-free, both in the review itself and “out of band”. So it seems worth addressing code reviews in a roundup themed on managing distributed teams.

I don’t normally point to podcast episodes here, but the SE-Radio episode on Code reviews with Michaela Greiler is worth listening to. Greiler has worked at Microsoft and helped analyze their literally decades worth of internal code reviews, and so has some really important insights:

Being clear on the expectations of code review is super important. What’s in scope for a review, and what’s not?
Reading code to find bugs isn’t super effective; tests are way better. (They have data!) So it’s better to focus on things that you can do best as a human reviewer: making suggestions about readability, maintainability, consistency with the rest of the code base, identifying new special cases that should be tested for, and the like.
Similarly with code style - just automate checks for things like that.
No “scope creep” for PRs: “while you’re in this file, you should fix this thing over here”. Review should be limited to the proposed change by the PR.
Similarly, debating whether this PR is a change worth making isn’t in scope for review; there should be some other process for having determined that.

Writing these things out sounds suspiciously like process, which research computing teams hate; but paraphrasing Michael Lopp, process is (or should be) just documented culture: “this is how we do things here”. It’s only when the whys of the process get forgotten and the process becomes a goal in and of itself that it starts to become onerous.

King’s article makes the same points about clarity of expectations, and goes into more detail about a specific process and where having an actual in-person meeting might be useful. I particularly like the addition of being clear on what “done” looks like for both a code review and the underlying PR.

The Google best practices for reviews are useful too if you want to see how a large organization that writes a lot of quite successful code goes about it.