Thu Jul 13, 2017
I’ve seen three different iterations of “code review culture”, and all of them have been positive with minor tweaks and changes. What follows is a general list of observations and advice:
“Finding and preventing bugs” is a secondary goal of code reviews, not a primary one.
You shouldn’t be relying on your code reviewers to find bugs; it’s nice to have an extra set of eyes who can point them out, but it’s your job as the implementer and tester to ensure correctness.
Everyone and everything should be subject to code reviews.
An atmosphere where senior engineers don’t have to go through CRs is bad: it implies poor things about your development hierarchy, like the idea that seniority means your code is immune to critique.
An atmosphere where critical bug fixes don’t have to go through CRs is bad: it implies you don’t have a testing/rollback framework set up to avoid the need to push unreviewed code.
Code reviews should not be more than X lines of code.
Anything more than X and diminishing returns set in. What
X is varies from language to language and context to context, but 200 is a pretty good number. (If your immediate reaction to this is “but most changes are more than 200 lines of code”, my immediate reaction is: nah.)
Super-basic style comments should not be in code reviews.
Like, they will be — someone (you!) is going to leave out a semicolon or mess up spacing, and that should be addressed in a review comment.
But if that’s the norm and not a occassional slip-up, then that’s a sign that you don’t have a documented code style. Whenever you or someone else comments we prefer null coalescing to ternaries here or whatever, that comment should go into a wiki and a lintfile so such issues can be automatically detected. 1 The goal is to avoid work that can (and should) be automated so reviewers can focus on the important stuff.
(Automation is always preferable to documentation: it’s nice to have a central resource for these things, but it’s much nicer to have that central resource be your IDE or a pre-commit hook.)
Don’t be an asshole.
People have a wide range of how they internalize code reviews, and it is often hard to separate critique of code from critique of the coder. The best way to do this is to approach code reviews not as an adversarial process, but as a collaborative discovery of the best possible implementation.
A great way to do this that some folks gloss over: write positive comments in CRs. Positive reinforcement is just as helpful as constructive criticism: praising someone’s tests or the way they structured some tricky business logic doesn’t just make them feel good, it enforces that behavior for themselves and others.
The largest personal and institutional grievance folks have with code reviews is that they gum up the works and increase the time it takes to deploy code. Set team-wide practices on healthy expectations for when code should be reviewed and stick to them.
The woooooooorst thing to experience as a review-ee:
- You post some code.
- A reviewer makes comments A, B, and C.
- You address those comments and post the new code, asking for a new round of review.
- The same reviewer makes comments D, E, and F, all of which could have been made during the original review pass.
The woooooooorst thing to experience as a reviewer:
- Someone posts some code.
- You make comments A, B, C, and D.
- They address only A and D and post the new code, asking for another round of review.
Keep at it!
Code reviews can be long and they can be difficult but they are good for the long term health of your code base, just like building out a test suite or documentation.
Some further reading:
- The great article on code reviews that inspired this post.
- Design Pressure, which describes a generalized framework about how optimizing your code for implementation-adjacent tasks. A lot of these observations, you might notice, are nominally about code reviews but really about broader development practices: that’s design pressure at work.
- Phabricator’s docs on writing reviewable code, and similarly their docs on crafting commits.
Thanks to Kelly Sutton, Iheanyi Ekechukwu, and Chad Little for reviewing this post. (And thanks to Michelle, Daniel, Andy, and countless others for reviewing my code over the years, and teaching me how to review well. 2)