Code Review Best Practices – Lessons from the Trenches

In response to this article's discussion of automating some tasks away, back when I was at Microsoft we studied the effects of an automated code reviewer on team collaboration. Our automated code reviewer utilized a family of analyzers (e.g., static, dynamic, binary, security, and dependency analyzers, along with best practice linters), unit test results, and feedback from the build system.

To summarize, a lot of effort during code reviews is spent on superficial details while missing high-level design discussions and bugs. Anything that can get rid of those superficial problems (i.e., linters and program analyzers) is helpful, but we found that it was even better to optionally show the analyzer's warnings to the entire team , not just an individual prior to the code review. Showing the warnings to the entire team let them discuss them (and they often did) and decide as a group what should be done.

Findings from our project: https://web.eecs.utk.edu/~azh/pubs/Henley2018CHI_CFar.pdf

There are a lot of articles out there on this subject (as this article points out). I think it's increasingly rare to find companies that can't see that value in code reviews.

What I'd like to see instead is some surveys of companies doing reviews to find out what's hard. What parts go well, what parts to they struggle with.

For example, at my past employer, we tried to pull in a privacy expert on any privacy sensitive changes and a security expert on any security related change. But this was really hard for two reasons. First, it's often not obvious that what you're doing is security/privacy related and probably the worst f-ups will be the one where you didn't mean to do anything security/privacy related. Second, it was hard to scale these "experts" who were domain experts but not an expert in the codebase.

I'm sure there are many many other things teams find hard and I think that's really interesting to dig into.

Devs are bad at code review, in that devs are bad at being fair, thorough, appreciative, clear. All too frequently code is "reviewed", when really it's just "marked as okay or not". Also, all too frequently a reviewer leaves a comment without concise actionable feedback - "You could do this this way" or "Sometimes I do ____ this way."

Style comments are almost always a huge pain. Style shouldn't be up to the dev most of the time - it should be an automated step as part of either dev or build. If it's not, don't comment on it within the review. Make a task to automate code styling instead.

If the author of the code doesn't immediately know what has to happen to get their code merged, the reviewer has failed. If the outcome of the code review requires changes to the fundamental approach that the author took, that is also a conversation that should not happen within the confines of the code review (because devs are bad at being concise, fair, clear).

If there is a culture where leaving positive feedback ("Looks great!", "Smart decision doing ____", etc) is strange, and PRs are used to overly scrutinize peers to appear greater than, find a new job. Don't even bother trying to help.

Related to the article: I noticed a lot of style like nits that ESLint didn't cover, but also weren't necessarily worth pausing and writing an entire ESLint rule for.

So I wrote checkr: https://github.com/ELanning/checkr which lets people plugin simple ad-hoc regex check lints (and underlines them in VS Code + git pre-commit check).

Hopefully to help break the endless cycle of code review nits that may be specific to your project.