Creating a code review process to ensure your team ships its best work

If your team has been interested in trying out new AI-based code generation tools, it’s a great time to make sure you have a solid code review process. Regardless of the code’s source, code review is how we ensure the work we’re shipping is reliable, appropriate, and can be maintained by the team. Here’s some key points to start you off.

Have an acceptance checklist

The first thing is to make sure that everyone follows the same outline when deciding whether a PR is ready to approve. This could be fairly simple: have someone who’s familiar with that section of the code read through the diff and wait for CI to finish running so you know the tests pass. But your team might have more complex design, documentation, or testing requirements. Here’s some other things that might appear on your checklist:

• All documentation has been updated
• New features have a corresponding design doc or RFC
• Bugs are linked to the bug report in the issue tracker
• Screenshots or other demo materials are in the PR description, so the reviewer knows what to expect
• Test coverage is stable or improved
• A changeset or release notes are included

You can make it easy to remember the steps by putting it in a pull request message template so every new PR starts with your checklist at the top.

Let your tools do the tedious parts

Expecting someone to pull your branch and run tests locally would be pretty silly these days when we have abundant CI tools available. Most organizations have some kind of automated test runner, such as a GitHub workflow. Other tools like linters can sidestep arguments about tab size, optional semicolons, and other formatting details. Automated tools can also help with documentation tasks, like compiling inline docs comments into a publishable format. Find the parts that a machine will never get cranky about having to do over and over, and automate that.

Be kind

Code review can be fraught at times. I know folks who are really anxious when they submit PRs because teammates have given harsh and poorly delivered criticism in the past. It can also be hard on the reviewer: how do I make sure they’re doing a good job without sounding like a jerk?

I love how the play Sumana Harihareswara and Jason Owen presented at PyGotham lays out the conversations we have around code review. (I saw them do a version of this at RubyConf 2018.)

A painful scenario

Jason: Wait, I thought HttpClient was the new thing?

Sumana: Who told you that?

Jason: Randall, when he reviewed this code. I just changed it to HttpClient yesterday!

Sumana: What? He .... [pause] So, there are a number of component choices, including this one, that are getting decided by the Architecture Committee, I’m sorry, I thought we’d already made a decision on this one, but this one’s for the meeting next week. We need to do a bake-off to see which one we want to use.

Jason: Really?

Sumana: I know. Look, ApiClient is significantly better than HttpClient. In fact I can use your branch as an example .... but if you just switched it ... Anyway, really we can't merge this PR until that's finished.

Jason: When will that be?

Sumana: Um... let's see, they meet every other week, the bake-off starts next week, Mike is out for two weeks after that so no meeting, so... 5 weeks, it looks like.

Jason: 5 weeks!?

Trying again with more collaboration and empathy

Jason: And I figured, while I was doing that, clean up the code a little bit, create a Bug class that can take care of more of those interactions, and of course add a test.

Sumana: Mhmm. Ah good, you said all that in your commit messages, too.

Jason: Yeah, Sarah was big on commit messages. She also helped me split up a commit I'd done before she came over.

Sumana: Good! These are great; tiny commits are so easy to review. Oh, it looks like the build finished; let's take a look.

Jason: Okay.

Sumana: Seems like there are a few style guide violations the linter caught; those should be pretty easy to fix. We have some docs on the wiki about how to run the linter locally; I'll send you a link.

Watch the whole thing. I think you’ll laugh and cringe at some of the examples and also find ways you can use code review to improve not just the code, but the team as a whole.

Validate the results

Finally, make sure you loop back and see how these practices are working for you. Talk about it in your sprint retrospective or planning meeting, both in the first weeks you’re following the new process and also after time has passed. If a bug lands in production and someone asks “how did this get past code review?” make sure you spend some time looking at why it was overlooked. It might be a simple oversight, or maybe there’s a step that wasn’t very clear and it turns out half the team was doing things one way, while the other half had a different interpretation. 

You might even try to do some calibration: have more than one person review the same PR independently, then compare notes. I like this better as a collaborative process though. Pair up with someone to review a PR together, and talk through what each of you notices. It can also be great to get on Zoom with the person who wrote a PR and have them guide you through the code, especially with big changes or when you’re reviewing an unfamiliar section of the codebase. You’ll learn more about your teammate’s work style and thought processes, and they’ll gain a better understanding of your review style.

Ultimately, code review shouldn’t be adversarial or a land mine. This is a process the team uses to create better code, stay on track, and be kind to future maintainers – especially yourselves!

All images have been obtained under the creative commons license from WOCinTech Chat Flicker