Let’s talk a bit about a big part of my job today. I currently work at Automattic where I help maintain the Jetpack plugin. I spend most of my day on GitHub, where I guide others as they contribute changes. I spend more time reviewing others’ code than writing my own these days. And that’s okay! I enjoy it a lot. Let me tell you a bit more about my code reviewing process. Let me know what you think about it, and don’t hesitate to share your own experiences and tips in the comments!
What is a code review?
Everyone has different expectations about what a code review should be. Where you work, the codebase you work with, the experience of your teammates with code reviews in the past, … It can all impact what code reviews will look like for you.
At Automattic, we ship different products in different environments. Your code review experience will depend a lot on the codebase you work on.
- There are parts of our codebase where we rely a lot on a ship -> iterate strategy; you commit a change without a review, listen to feedback from users, from support, from colleagues, and commit a follow-up to iterate based on that feedback.
- There are also repositories where we rely a lot on code reviews. In the Jetpack repository, code reviews are actually mandatory before you can merge changes and get them shipped in the next release.
All that said, if I had to pick one word to define “code review”, I would probably define it as a conversation.
A code review is a conversation.
In most cases, a code review will be an asynchronous conversation, a back and forth between the PR author and the review, until both are happy with the changes and deem them good to merge.
While everyone’s experience may be different, I believe there is a way to set up a code review system that will work for everyone, with standards to ensure that your reviews are comprehensive, exhaustive.
Just like in any conversation, you should of course still keep your interlocutor in mind. Do your research so you know what their experience with code reviews is. Check if it’s their first time contributing changes to the project. This should help you tailor your feedback to each individual, while staying mindful of your potential biases.
In my opinion, we can set 3 main goals for ourselves when starting a code review:
- Ship good code (maintainable, free of bugs).
- Build relationships and trust between colleagues, get to know them and the way they work.
- A code review should also be an opportunity to learn for everyone involved; for the PR author, for the reviewer, but also for any reader that may stumble across that code review in the future.
If you ever found yourself in a “git blame” rabbit hole, you’ll know how insightful a conversation on an old PR can be to help you better understand a part of the codebase.
How to review
In this post, I’ll focus on the work of the reviewer: that is what I am most familiar with.
When we talk about code reviews, a few things come to mind first:
- Does it work?
- Can it be abused (are there any potential security vulnerabilities)?
- What impact will it have on performance?
- What are the gotchas with this part of the codebase? Is there anything we must keep in mind beyond the list of testing scenarios offered by the PR author?
These questions are all important, but they focus on the code. A PR is more than just code. Before we start looking at those things, I would encourage you to take a step back, and start at a higher level.
- What does the code impact? If we’re making changes to a package or a library, what will it change for the library’s consumers?
- Is this the right approach to the problem? The code may work, but there could be a better way to get this done. You may be able to leverage existing work for example, instead of reinventing the wheel.
- Is the code easy to understand?
- Does the code match our standards?
And finally, two wrap-up steps that I think are worth keeping in mind:
- How will the code be maintained? Who will maintain it? This may impact your code review, as you may want to do things differently if maintenance is going to be light or done by a different team not familiar with the codebase.
- What’s next after the code review / merge? Are there some support documentation updates needed? Will other teams (support, marketing, …) need to know about those changes?
When diving into a code review, I try to make multiple passes at the code, focussing on a different step each time. If I start at a high level, and then dig deeper into the PR as I go, it helps put myself in the right mindset for the review.
Ok, that’s a lot of things. So what should you remember about all this?
Do not start with a code review.
As I mentioned earlier, you’ll do yourself a favor if you take a step back before you dive in. Code reviews are helpful, but they’re often not the best place to start talking about changes.
Depending on how things work at your company, you probably already know this: project boards, project scope documents, technical spec discussions, … This is where the code review really starts, and this is background you must have to do a good code review.
If you don’t have that info, ask for it. Get out of the code review context to get more information about a change, to gain perspective about the reasons behind that change. That should inform your review and set your expectations.
Pace yourself
Reviewing is tiring. You don’t want to get into a situation where you’re less careful, where you are too quick to approve because you can’t give it your full attention anymore.
This is also why scoping is important. Ideally, the PR author will keep you, the reviewer, in mind as they work on their PR. It may make more sense to break things into multiple PRs. Introducing new things one at a time will be easier to test, to review, and you will be able to do a full, proper review of each set of changes.
If that doesn’t happen, you may end up with PRs that are a bit harder to work with.
Alright, alright, I hear you. “Ok Jeremy, can we actually start with the review now?”
Well, there is one more thing…
Consider your skills
Are you the best person to review this change? Being a reviewer can also be pointing the PR author towards other reviews. Maybe someone has a better grasp on that part of the codebase than you. Maybe you’d benefit from a security audit.
In any case, it can be useful present your review as “superficial”, when you’re not the best person to review this code / codebase. Just say so as you review, and help the PR author find the person that can best help them.
Let’s start reviewing
Let’s look at a specific aspect of code reviews. We can talk standards, style guides, linters, … More generally, consistency.
Style Guide
A good way to set up clear expectations for every contributor is to create a style guide. Once you have a set of rules, written down for everyone to see, you can avoid arguments on code reviews. The rules are already set, so there is no need to come up with rules with each new code review.
That said, you don’t have to start with a full-fledged style guide on day 1. This is something you can build over time, with the help of code reviews. Every time you find something contentious during a code review, and every time you come to an agreement about a new rule, write it down in your style guide. If you continuously iterate on your style guide, every contributor knows about it, and you’re ensuring that it stays up to date over time.
Linters
This brings us to linters. Having a set of rules is great. Enforcing those rules is another problem.
You could spend a significant amount of your code review time making sure that all rules you’ve set up for your codebase are respected. That wouldn’t be an efficient use of your time.
Instead, let machines do the work when possible. This has 3 main advantages:
- The linters give feedback early, before any human has even looked at the code. The PR author can make changes without having to wait for your review.
- You can focus on all the other, more important, things we mentioned earlier when talking about code reviews.
- The PR author won’t be frustrated with you. They may disagree about a specific rule, they may find some of the guidelines to be annoying nitpicks. They’ll be upset at the linter for all that, instead of getting upset with you come code review time.
Let the PR author be frustrated at the linter, not the reviewer.
Clear expectations and tools to enforce expectations allow us to save time.
Standards and consistency
Standards, and the tools we mentioned earlier, help you have a more consistent codebase. In practice, this means:
- Much easier onboarding (new contributors can look at one part of the codebase, learn, and copy).
- It’s easier to skim through code, whether you’re making changes to the codebase, reviewing code, or debugging issues.
- The codebase becomes easier to maintain: one change can be quickly replicated across the whole codebase when there are similar patterns.
Enforce that consistency in your code reviews.
Consistency will make your codebase easier to understand, to read. Having code that works is important, but understanding the code is really something you should look for when reviewing code.
Any fool can write code that a computer can understand. Good programmers write code that humans can understand.
Martin Fowler
To that effect, don’t be afraid to ask questions.
Questions help you, and they help others. It may feel a bit embarrassing to ask questions at times, but doing so will benefit everyone.
If something isn’t immediately clear to you, it may not be clear to others, and it may not be clear to anyone (including the author themselves!) a year from now.
Your question may lead to adding comments in code to clarify things. It may even lead to some refactoring, but at the very least, there will be a conversation that will be available to anyone blaming that code in a few years. That conversation may provide important answers to someone trying to understand the reasons behind a change.
Testing
We’ve talked about the code, but we must not forget one integral part of the code review: testing.
When testing, there are a few things to keep in mind:
Be exigent.
Ask for detailed testing instructions. You should be able to test the changes by just following instructions.
This is why PR templates are important. They make it easier for you, repository maintainer, to set up clear expectations so they know that they’re expected to add detailed testing instructions to their PR.
Get out of the testing scenario.
This is where your knowledge of the product is important. As we discussed earlier, an important part of the code review process is to understand the potential impact a change may have: what relies on that part of the code, would they have to make changes on their end following your PR?
When testing, try to think outside the box so you can test scenarios that were not proposed by the PR author.
Giving feedback is hard!
It is!
Telling people what they did wrong is never easy. There is no reason for it to be easy in a code review either. If you “just” give someone a list of 50 things they did wrong, they are not going to be in a great mindset to address that feedback.
We consequently have to think carefully on how to best deliver that feedback. I can think of a few things we can do:
- Don’t just nitpick.
- Be generous with your code examples.
- Do not say “you”.
- Be more polite than IRL.
- Give suggestions, not orders.
- Your opinions are not facts.
- Be clear about what’s important.
- Offer praise.
- Handle stalemate proactively.
- Take a look back at your comments.
Don’t just nitpick
Keep your feedback varied, and don’t just focus on one small part of the PR. You looked at that contribution from all angles: show it in your review, and comment on all the things you reviewed.
Be generous with your code examples
When leaving a review, you want to show that you’re here to help. You’re not there as a blocker, you are working with the author so they can get their changes in.
To that effect, don’t be afraid to share your ideas and comments with code when possible. Instead of a short comment suggesting that “you could do this instead”, you could show them an example of how they could do this.
This is especially important when suggesting small changes or nitpicks. Make it as easy as possible for the PR author to address your feedback and move on.
This is especially true if you use built-in tools like GitHub suggestions. They allow you to suggest a code change while reviewing. The PR author can then click one button to commit your suggestion in their branch, without having to write the code themselves.
If you use a platform that doesn’t have a built-in suggestion tool, you can always try some changes on a local copy, save those local changes as a diff, and then suggest that diff in your review. The PR author will be able to apply and commit it with a few clicks.
Going the extra mile in your review comments turns the review into a positive experience for the PR author. They will understand that you’re on the same team, you’re here to help and put in the work to make that change happen.
Do not say “you”
When reviewing, let’s avoid blaming one person. Just like above, show that this is all a team effort. Everyone should feel ownership over the changes that will get merged.
If you’re suggesting a change, say “we” (“Could we rename variable x?”) or use the passive voice. It may sound silly at times; after all, the author will be the one doing the work. Still, by doing so you’re going to sound less aggressive, you’ll remove the personal element from the discussion.
Be Polite
This sounds obvious. It should be! Yet, we must remind ourselves, whenever communicating online, that we must be even more polite than in real life.
In your review, you’re interacting with a peer, not a subordinate. Avoid sounding bossy, to set the right tone in your interaction.
And just like when chatting online, don’t hesitate to use emojis to convey emotions and help carry tone alongside your comments! Emojis always help! 🙂
Give suggestions, not orders
I would recommend framing your comments as suggestions, or requests, instead of an order: “Could we rename variable x?” sounds softer than “you should rename variable x.”
More importantly, when you ask a question you leave room for discussion.
Leave room for discussion, for the PR author to say no or to punt to a follow-up PR.
The PR author will have the chance to explain themselves, to push back on your suggestion. It’s a lot easier to argue about a suggestion than to push back on an order.
Your opinions are not facts
Likewise, sometimes you may share an opinion on a code review. When you do so, make that clear. Use “I think”, “in my opinion”. It will frame your comment so the PR author can continue the discussion accordingly.
Be clear about what’s important
You may leave 20 comments in a review. Some of those comments may be important problems, things you would consider a blocker for the PR to get merged. Other things may be opinions, as discussed above, or may be tiny changes you’d be willing to let go.
Make that clear in your review, so the PR author knows what action they must take for each comment.
Offer praise
Code reviews are not just about finding bugs. They’re also a learning opportunity for you, the reviewer.
If you like something, say it.
If you’ve learned something new, say it.
Similarly, it’s always good to recognize improvement. Mention past contributions in your comments, and offer kudos when you appreciate a change:
- “This is going to improve what we had done in this PR so much, thank you!”
- “This is a much better approach than the last implementation.”
Handle stalemate proactively
Just like with other conversations, sometimes things don’t go as planned. Following a review, the author refuses to make changes and you’re not moving forward.
It’s important to recognize the signs of a stalled conversation as early as possible:
- The number of comments you leave doesn’t decrease with each review.
- The tone changes, becomes more aggressive or passive aggressive.
- You get push back on a lot of your comments.
When this happens, I would recommend a few things:
- Take it out of the review context. Can you chat with the PR author about it? A synchronous conversation can sometimes clear simple misunderstandings that may have grown out of proportions in the review context.
- Take a break. Wait a bit before your next review round. Sometimes, “sleeping on it” is the best thing you can do. It will help you approach things with a fresh eye.
- Take a step back. At times, it’s worth having a larger conversation about the changes, in another forum, where other folks and colleagues can get involved.
Once you’ve considered all this, and if that doesn’t unblock things, it’s sometimes also okay to “disagree and commit“.
If you encounter those issue in a work environment, this can be a good time to reach out to your team lead, so they can help you deal with this:
- They’ll be able to reach out to the PR author and act as a mediator if needed.
- They may be able to help you take a break from reviewing PRs, to help calm things down with your colleague.
- They’ll ensure that the conflict is resolved.
Take a look back at your comments
Before you hit that “Send” button, it’s important to take a final look at your review:
- Is it balanced? If you have a dozen nitpicks and one comment about a bug, that one comment may get lost.
- If you’ve looked at all the levels behind a change, as we discussed above, your review should reflect that.
- If you did not have the bandwidth to do a full review, mention it so the PR author knows what not to expect from your review.
Over at Automattic, we follow this creed. I think that one sentence of the creed applies very well to code reviews:
I will communicate as much as possible, because it’s the oxygen of a distributed company.
Automattic Creed
I try to live by this creed every day, and I would encourage you to think about this as you work on your code reviews!
This was a long post. Congratulations, you made it to the end! Please let me know what you thought about it. You can leave a comment below, or get in touch via email. I’d love to get more tips to improve my code review skills!
Thank you for reading. 🙇♂️
One reply on “The Power of a Conversational Code Review Culture”
This was mind bogglingly good, which is no surprise, coming from you. :)