Growing a Code Review Culture
Big Nerd Ranch esteems code review. We’ve seen it pay off time and again. It is core to our workflow and process. If you want to experience the benefits in your team, here’s what that means in practice for everyone involved.
Leaders Set the Stage
Leaders foster a culture of review as top priority. There are good reasons for this, as elaborated by Glen D. Sanford in light of their time at Twitter. Those reasons can be summarized as:
- Keep the cache hot: The longer it takes to close out a review, the more time everyone involved has to forget what the code being reviewed was even supposed to do. Relearning that takes time. Avoiding relearning saves time.
- Keep the cycle time short: The details of code are not the only thing you can forget. A review is a discussion. If that discussion becomes drawn out, you can forget the context of the discussion around the code, as well.
- Minimize open loops/balls in the air: (Choose your preferred metaphor bingo square.) As would-be changes stack up, there’s more chance for conflicts, and more might-bes to deal with. The faster a review process completes, the sooner you convert a maybe into fact.
Authors Must Remember Their Audience
Authors need to create PRs that are intended to be reviewed.
In practice, this means:
- Keeping your PR small and coherent: A small PR means there is less code to review; a coherent PR means reduces the number of potential concerns a reviewer might have by concentrating the effect of your changes.
- Keeping your commits small and coherent: When few lines are changed per commit, you get more opportunities to explain those changes through a commit message. The commentary–to–code ratio rises, which gives you more chances to make the changes easier for your reader to understand.
- Writing commit messages that tell a readable story: Commits do not happen in isolation. Your reviewer will see the commits in order from first to last. If those commits tell a clear story of how you moved the system from point A to point B, the reviewer will have a far easier time navigating and assimilating your changes.
- Contextualizing your changes: Git can tell your reviewer which files and classes you changed. It can’t tell them why you made that change. Sure, you renamed a parameter, but why? How did you choose the new name?
- Keeping code clear: Clear code is easier to read than convoluted.
It can be instructive to compare these principles to the SOLID principles. As with the structure of code, so with the structure of changes to that code.
Reviewers Are Co-Authors
Reviewers need to take the responsibility seriously. Review is an opportunity to have a lasting effect on both code and team.
In practice, this means:
- A reviewer must know they have that responsibility: Assign someone to review a PR. Don’t just cross your fingers. GitHub’s CODEOWNERS file-glob–based auto-assignment system can automate this.
- A reviewer should aim to improve both code and coders: Code review is a valuable way to share knowledge, best practices, and style standards. For more experienced developers doing review, this is an opportunity to teach something with a very concrete context. For less experienced developers doing review, this is an opportunity to ask questions with a very concrete context.
- Asking questions can be more valuable than giving answers: Getting a review that amounts to someone ghostwriting through you can be very frustrating. But a review that elicits an unseen problem through dialogue and solves it through collaboration is exquisite.
- A reviewer should focus on the issues only a human can catch: Code formatting and layout issues should be handled by automated processes. Let formatters/beautifiers, linters, and spell checkers do their jobs. Machines aren’t going to catch redundant abstractions, missing abstractions, inverted conditions, or mixed abstraction levels within a method’s implementation. Automating what you can reduces reviewer and reviewee burden and avoids drowning valuable review comments in a sea of noisy nitpicking comments.
Reviews Are a Canary for the Whole Process
If a team feels that reviews are rubber-stamps en route to landing changes, there will be trouble. Reviews will be reduced to unwanted busy-work.
If a team is planning work without allowing time for code review, there will be trouble. Reviews will be rushed. They might convert into rubber-stamps as a way to leave breathing room for other planned work.
“Done” includes a code review. If people feel there isn’t time to review work done, then they will be landing half-baked work. Taking on less work helps here. Kanban’s limits on work-in-progress can effectively require reviews be completed to free up space for further development.
(If PRs are piling up, you are headed for a headache of merge conflicts that everyone involved will have forgotten how to resolve, never mind review. That is a warning sign in and of itself, and it can emerge with or without a review culture.)
It’s also important for people to have realistic expectations about the time review can take. Worked three days on a PR? Expect it to take at least three days to review.
Or better, don’t work for three days before submitting something to be reviewed! The adjustment in perspective from “a PR finishes everything about something” to “a PR pushes the project to a slightly better state” can take time, but it also can unlock a lot of process improvements from planning to estimating to development and testing to, yes, reviewing. Issuing a PR each day keeps the chaos at bay.
- Leaders must lead their team to prize and prioritize reviews.
- Authors must not forget their audience. More, smaller PRs written with review in mind will accelerate everything.
- Reviewers must take co-ownership. Automation can free everyone’s time to focus on real issues.
- Review is the other half of development, and it can take as much time as development.
Interested in nurturing a code review culture in your organization? Reach out to Big Nerd Ranch today to talk about how we can work with your team to raise the bar.
Thanks to my colleagues who precipitated this post and contributed content and feedback: Josh Justice, Dan Ra, and Evan McCoy.