Collaborating on Code Review

20 Jun 2023 · 📖 in 4 minutes A short reference for writing great pull requests

I was chatting to a friend recently about what makes a good collaborator and the pitfalls you run into when working with a less experienced team. Rather than enumerate all the things folks might do wrong I figure it might be more constructive to write up a short reference for writing good changes for code review.

Whenever collaborating it's always good to think about your audience. What context do they have? Are they in your team? Are they busy or stressed? With these things in mind here are a few suggestions that we can do to make it easier on folks who are reviewing your changes.

Clean Commits

Write a solid description

In most cases you as the author are deep in the change and while this gives you the most context it can sometimes be hard for reviewers to understand. Why you are doing the change and what you are trying to achieve? Answer these questions with a description and it'll help cue reviewers when they're reading over your change. Bonus points for linking to a technical design doc or change ticket (e.g. JIRA).

Keep it short

It's likely that if the change is reaching across many different files, components, and libraries - if so it's a good candidate for breaking up and pushing as a series of dependent changes (see ghstack for a way to do this in Github). This is a good habit to practice in most cases as shorter commits often represent a smaller, easier-to-reason-about, part of an implementation.

Self-review

Occasionally there's cause for larger changes that can't be split up, in this case, it's always a good idea to "annotate" your change with a series of comments on lines that you feel warrant explanation. I've seen full video walkthroughs for some changes that required a decent amount of domain expertise and having a quick 3-minute walkthrough from the author helped me understand the context around the approach.

Follow up

People are busy, you are busy, and sometimes you'll start a review and then get distracted. If you're still waiting for that crucial piece of feedback on your change then walk over to their desk / slack them and politely ask for some of their time. It could also be the case that you just don't understand a comment made on your change, so rather than starting a thread on the review maybe have a chat and ask the reviewer to take you through it. This can be super useful if you have a change up for a long time.

Clear warnings and write tests

Write tests and clear warnings triggered by linters (though depending on your build pipeline it may be difficult to even push to review in an incomplete state). Any "noise" on changes will distract reviewers from giving you the feedback you're looking for. Similarly, if there's an expectation that your changes require unit tests then it can be quite tempting to "add them later" however they are as much a part of the code review and it can cause a change to need several rounds to get it merged.

Prototype freely

If you're still unsure about a specific path to take then a code review may also be a good way to gather feedback on an implementation before fully "committing". Most code review tools offer a way to share snippets or draft out a change that can go up for feedback but with clear signposting that it's not ready for merge. I'm a huge fan of this approach and I find writing a few different implementations, seeing how they work out, and checking for any unforeseen/unexpected behaviour can be super valuable in gaining confidence in your approach.

Be humble

It's normal to feel criticised (maybe even attacked) when that change you've spent days crafting has received large amounts of feedback or questions that question the underlying approach. Take a breath. Code review is an exercise in collaboration, it's you and the reviewers tackling a problem, and you might not agree but this is exactly what you're looking for when asking for review. That said, there's review feedback that can be noisy in itself, things like style/formatting that we can lint for. (ProTip spelling can be hard, esp when writing code, try Code Spell Checker.)

What about you?

These are by no means exhaustive suggestions and I'm sure you might have different ideas about what makes a good collaborator in this space so let's chat on mastodon!

First appeared on Trusty Interior, last update 22 Jun 2023