How do you do code reviews?
This question was recently presented to me from someone considering adding code reviews to his team’s process.
This post lists his questions and my answers (lightly edited) for the benefit of other teams looking for advice about starting code reviews.
I think this is the best place for automated linting tools for style guides and other static analysis. In my experience, having those tools in place removes 99% of pedantic debates. People are shockingly comfortable with obeying linting tools if that’s the team policy. Once those checks are in place, I’ve found that the conversation is generally elevated above pedantic commentary.
Beyond that, a cordial tone in writing review comments is critical. Asking questions instead of speaking in absolutes often changes review conversation dramatically. The key is that the team remembers that reviewers should be seen less as gatekeepers and more as collaborators with the intention of improving the code.
I push back on code when there is a clear defect that will cause problems. Otherwise, my policy is to leave my comments and approve the code review, trusting that my teammate will consider the advice and apply any appropriate changes. The combo of “here are some things that I think could be better” plus “I approve this” acts as a signal of trust.
At Storybird, we’re extremely wary of future proofing because the future is so uncertain. There is some appropriate place between a hack and gold plating. We usually accept something and improve the code in a future branch. Again, static analysis may save your bacon here. Adding checking for cyclomatic complexity and the like sets the bar in case someone tries to commit something truly awful.
I think developers still learn tremendously when code snippets are given.
I will often put in code chunks as a sketch of the real thing.
The core bits might be there,
but I don’t try to compile the code
and I might do something like add
# Do the rest of the stuff comments
to laser focus on the code that really matters for discussion.
I’ve done both styles and online versions are vastly superior. The async nature of a code review respects the developer’s time. Our profession is so focused on getting into flow. If you have to show up to an in-person code review (physical or phone calls), it really ruins those chances to get into flow in a day. This would be especially true if you end up having multiple reviews per day. I think you’ll have much easier buy-in from developers if you go with the online version.
My rule of thumb is a 500 line diff max. Too much beyond that and even the best reviewer’s eyes start to glaze over. This favors delivering small features or clear chunks in a feature. Since we deploy often, we merge large features behind feature flags.
There are some occasions where we’ll review large branches. We try to make this rare enough that a developer feels slightly guilty for making a huge branch. I don’t suggest that this guilt is for shaming. It’s more of a “shoot, I made my teammate’s work way harder by giving them tons to review.”
We usually have code reviews (GitHub Pull Requests) scoped to a feature level. By scoping to a feature instead of a single commit, it’s a natural transition point to move onto some other task while waiting for a review.
On the occasion that I’m working a really large feature, I might start from a feature branch (branch A) and put up a pull request when a decent chunk of work is done (like a sub-feature in the overall picture). Then I’ll branch from A to create branch B and continue working while A is under review. This allows branch B to use stuff that it might need from branch A. When A is done and merged back to master, I’ll merge master back into branch B. Doing this makes the PR for branch B only show the content for branch B.
I hope some these answers can help a team that is thinking of starting code reviews. If there are other questions that aren’t answered here, feel free to reach out, and I can try to answer.
If you want to chat about this with me, I'm @mblayman on Twitter.
How do you get into the rich field of data science and the array of tools that are available in Python? Christine Lee provided an excellent overview of the data science ecosystem. The talk, from the December 2017 Python Frederick monthly event, is full of goodies to check out.
Matt is the lead software engineer at Storybird.
Always eager to talk about Python and other technology topics, Matt organizes Python Frederick in Frederick, Maryland (NW of Washington D.C.) and seeks to grow software skills for people in his community.