December 08, 2023
I spend a good deal of my time reviewing code written by my teammates and peers. At this point, I probably review more code than I write.
Here’s how I think about code review:
Earlier in my career, I saw code review as a tool for finding and fixing problems. Code review, I thought, prevented us from shipping bugs or sloppy code.
With that problem-seeking mindset, I’d search for problems in the code and point them out to the author. Once all of the problems were resolved, I’d approve it.
This problem-based approach only works as well as your understanding of the code and its impact on the broader system. There’s a good chance the code has some problem in it that I simply can’t see.
Over the years, I’ve slowly shifted towards seeing code review as a tool for building a shared understanding among the author and their peers.
With this understanding-seeking mindset, I ask whether I understand the purpose and reasoning behind the change. If not, I ask questions. Once I feel I truly understand the change, I approve it.
When I start a code review, I’ll usually open up a blank document where I can explain the change to myself in my own words. As I do this, there are a few questions I use to guide this process.
If I can’t answer these questions during my own review, I’ll ask this in writing in the pull request so the conversation is visible to the wider team.
Remember, if something is confusing to one person, it is very likely confusing to other people, too.
Why do we need to make this change? This should be very obvious, either in the change description or in some attached ticketing system. If it’s not obvious, ask why.
Asking this question can often stop an unnecessary or misunderstood change.
Now that I know the purpose of the change, I can look at every line in the change and ask how it ties back to the original purpose.
If there are changes that are unrelated to the original purpose and are not obvious improvements, ask why.
If some aspects of the motivating problem are not addressed by the change, ask why.
It should be obvious to validate that the change accomplishes what it sets out to do, either through automated testing, manual process, or metrics. If not, ask how we really know it works as described.
Now, I can get into the actual implementation. Sit down with a beginner’s mind and ask: “How does the code actually work?”
Can I follow it, line by line, and explain it to myself or someone else?
If I can’t understand it, try to understand why. Where exactly am I getting stuck? What about that section is difficult? Am I missing context about the problem or knowledge about a particular library/function/technique?
If I don’t understand it, it’s very possible that it’s actually just a me problem and not a problem with the code! Ask public questions to clarify. This will help other reviewers.
Assuming the change does what it needs to do and is relatively easy to understand, does it introduce any new complexity to the broader system?
For example, imagine the change under review implements a call to an external API, a very standard thing in web application. The codebase already has several similar API integrations.
The change does exactly what it needs to do, but it introduces a new, faster library for handling the HTTP request.
On one hand, this could be a small improvement (the new library is faster!). On the other, now we have two ways of making HTTP requests. This makes the overall system slightly more complex.
Is this added complexity worth it? Maybe. Ask about it and start the conversation. The team should discuss this and come to some consensus.
This is all the lower-level stuff that every change should do well. Ideally, some of these are caught by automated tools, but check whether …
The more code I’ve reviewed, the more I’ve appreciated the extra effort that some authors put into making their change easy to understand.
Every reviewer of your change — and every future developer who touches your code in the future (including you) — has to expend some energy to turn the characters on the computer screen into some useful understanding in their brain, a process called interpretive labor.
There’s a tradeoff between the energy put into explaining an idea, and the energy needed to understand it.
On one extreme, the explainer can painstakingly craft a beautiful explanation, leading their audience to understanding without even realizing it could have been difficult.
On the other extreme, the explainer can do the absolute minimum and abandon their audience to struggle. This energy is called interpretive labor.
— Olah & Carter, Research Debt
If you submit a large, complicated change without any context, you are demanding the maximum amount of interpretive labor for every reviewer.
Alternatively, you can take on the interpretive labor as the author and make the change easier to understand for every reviewer. A few helpful introductory comments from the author can drastically reduce the interpretive labor required from reviewers.
Some things you can do to reduce interpretive labor:
Many people who are much smarter and more thoughtful than me have shared their thoughts on code review. Here are a few I liked:
