You can tell a lot about an engineering team based on how they conduct their code reviews.
From my own experience, no two code review processes look the same. That can be because conventions for how teams do their code review tend to form organically. As conventions, they tend to be ad-hoc, informal, and transmitted from person to person. As the team grows or its makeup changes, expectations can start to differ.
At Thread, this was no different. Whist we had clear guidelines for how to get your code reviewed, for example, how to tag individuals for review, we didn’t set out clear expectations for both reviewers or authors, such as what tone to aim for when communicating, and what to consider when submitting feedback.
After putting together our code review guidelines, I wanted to share them publicly. This has worked well for us as a team, however no two teams are quite alike, so your mileage may vary.
We believe code review serves the purpose of improving the overall quality of the codebase. Code review achieves this by sharing knowledge throughout the team and improving our craft as engineers through learning and peer review.
Consider this example. As a reviewer you might spot a bug caused by an edge case and comment on it. You suggest a test case that highlights the bug. The author acknowledges it, writes the test, fixes the bug, and the code is shipped.
As a reviewer, you stopped a bug from reaching production. That’s beneficial, but not the only reason to do code review. In the future, the author is more likely to consider similar edge cases. They might have learnt something about good test cases to write, and so are going to write higher quality code. In the long run, we’ve improved the overall quality of our codebase through teaching and self-improvement.
The guidelines given here are exactly that: guidelines. They’re not rules. It’s important to acknowledge that on rare occasions it may be necessary to go against the guidelines.
Conversely, the guidelines may force us to challenge our behaviour and instincts when we approach code review. Going against the guidelines might feel tempting, but won’t help us change our habits over time.
The process of code review should be lightweight, it should not be a bureaucratic hurdle to clear.
At Thread we value being able to rapidly ship features and experiments. At first glance, having a code review process might seem at odds with this.
However, in the long run, the fastest way to ship is to have a high quality code base. You might want to read our approach to product development, called Test & Delivery, which also touches on this topic.
This tension can occur because it’s tempting to see code review as a set of rules. Seeing code review as rules set in stone misses the point of why we do code review.
Coding is a creative exercise that is as much a craft as it is a technical activity. Given recklessly, criticism of a piece of code can feel like criticism of a person. As a result, we must acknowledge that every submitted pull request involved hard work and effort from the person behind it. Nobody intentionally submits poor quality code for review. It’s our job as reviewers to start from the perspective of wanting to help our fellow engineers, and a desire to achieve mutual understanding.
As a reviewer, you could be lacking context. Be open to the idea that there might be a good reason behind a decision you disagree with, so try to start with questions rather than judgements.
It’s better to lean towards offering concrete suggestions for improvements rather than simply criticising, e.g.: “I found this function hard to follow, could you split it up into smaller sections?”
Leaving snippets of code is a much clearer way of communicating your suggestion. It might seem clear to you what you think the solution looks like, but it’s often easier for an author to digest a comment when paired with an example.
Many aspects of writing code are subjective. When exactly does a unit of code have too many responsibilities? When is the right time to split something apart? The answer is nearly always both subjective and contextual. As a result, blanket statements in code review such as “this class has too many methods” or “this should be a generator” are rarely helpful.
Start from your experience. If you found something hard to follow, or you were surprised by something, say so.
“I’m finding this class hard to follow because it has too many methods”, or “I think this might read better if it were a generator”.
Minor nitpicks or deviations from a convention or style guide should be highlighted as such. It’s important to be able to distinguish between these kinds of comments and any larger concerns, and it helps the reviewer with understanding relative priorities when addressing comments.
In some cases, you might comment with a suggestion, but not feel strongly about it. It’s useful to say so in this case. This is so that the author knows that you don’t necessarily expect it to be implemented, and are just providing an idea or alternative point of view.
In the general case, it’s good to leave expectations clear. This helps keep the code review process moving. The author should know whether they have comments to address, changes to implement, or if they’re ready to merge the change.
Code review is not the time to introduce new preferences, patterns, or conventions for writing code that are not shared by the codebase and the team. Instead, we bias towards following existing conventions and patterns. We strongly favour uniformity across the codebase. Further, if older code doesn’t meet accepted newer best practices, it might not be advisable to update it right now. Often, the effort to refactor an area of code might outweigh the change itself.
However, it can be valuable to mention where you feel the could be improved in this way, in order to start a discussion, but keep your expectations of the author clear.
Thread encourages individuals to run experiments to try better ways of working. This is just as applicable to our codebase as it is our working methods.
Many minor refactors might make a piece of code better, but taken together they can be very time consuming. It’s important to consider how the time of the reviewer and author might be spent. Lots of code at Thread does not have an infinite time horizon. We run lots of A/B tests of which many fail. It’s not uncommon for areas of the code to go through many independent iterations.
At Thread, We use GitHub’s Pull Request (PR) and code review workflow. It’s a fairly flexible system that doesn’t impose many constraints. Many teams develop their own conventions around how to use it. These are ours.
Comments should be read as suggestions not instructions. The author might reasonably disagree with the comment and choose not to implement it. This is fine, and authors should feel comfortable pushing back on comments they don’t agree with.
As an author, its expected that you address all the comments left on a PR. You might choose not to implement a suggestion, but you should reply to the comment saying as such, and explain why.
It’s recommended for authors to reply to a comment with the commit hash containing the fix addressing the comment. It’s helpful to reviewers to keep each commit specific to each comment, although fixing a collection of minor issues together in one commit is accepted, e.g.: spelling, grammar, style and linting.
Smaller PRs are easier to review and avoid the negative feeling that occurs when a large PR has a number of comments raised against it. It allows incremental progress that stops review feeling like a slow moving part of the process of shipping code. Sometimes, larger PRs are unavoidable. In this case, it might help to have a code review in person with the author, particularly if there is a lot of context for the reviewer to absorb.
Where this isn’t possible, we like to put extra effort into making each commit a single logical change for the benefit of the reviewer, so they can review commit-by-commit. Let reviewers know in the PR that this is recommended.
We prefer to keep PRs contained to their description. Refactors or tidy-ups are valuable, but can clutter the diff and make review more difficult. Uncontentious small changes like this can be cherry-picked onto master, and don’t require review.
We aim for the vast majority of reviews to be done in a single round. If an individual comment has generated a back and forth, this is a signal to follow up in person. Pull request comments are asynchronous, and tend to be a slower way to resolve issues.
In person discussions have better bandwidth, and make it easier to come to consensus. However they are not visible to other members of the team who may be participating. For clarity, record any decisions made in a comment.
GitHub allows reviewers to finish their code review in one of three ways. Approve, to do just that, Comment, just leave the comments as is or Request Changes, to signify the reviewer would like the author to make changes before the Pull Request is merged. We think it’s helpful at Thread to clarify further what the expectations on reviewers and authors are in each case.
As a reviewer, when giving approval, it can mean one of a few things. It’s helpful when finishing a review to leave a comment explaining which of these you mean.
You’re happy for this PR to be merged with no additional changes.
I’m happy for this to be merged, conditional to addressing these comments.
Your approval might be given conditional to addressing some comments. This is a good way of leaving the final judgement to the author and avoiding them having to wait for another round of review.
I’m not sure I’d have done it this way, but I’m willing to defer to you here and we can address concerns at a later date if necessary.
Approval should be given even if you have comments, but you would be fine with the code being merged pending the author’s judgement. They might choose to merge with some or all of your comments left unimplemented.
Just leaving a comment without either explicit approval or requesting changes can be confusing to the author, as expectations are less clear for the author. This might be fine if you’re a non-blocking reviewer (someone who the author is just soliciting an opinion from) rather than the primary reviewer. Try to avoid leaving just comments if you are the primary reviewer. Not sure if you are? Seek clarification. Not comfortable giving Approval? Why is that? Question whether you would be happy to defer and address your concerns later. If you’re not, follow up in person with the author.
Requesting changes should be used sparingly. You should use it when you have concerns you’re not comfortable deferring, and you want to review the PR again. This could be for a few reasons:
When a reviewer uses the Request Changes button, it can be a signal that some communication has been missed or there was an oversight when planning the work. As such it’s useful but not necessary to follow up with the author to understand why they approached the problem as they did. They may need help understanding why their solution has problems.
If a Pull Request has architectural problems, intervening at the point of code review is costly, when a lot of work has been committed already, so retrospect on why this happened.
We want our code reviews to be cooperative, rather than adversarial. By sharing these guidelines in the team, we’re able to have more productive code reviews, encourage a high quality codebase, and develop our skills as individuals.
If you like the way we do code review, you may also be interested to know that we are hiring! Check out our jobs page to see the open roles we have and apply.