Apply these strategies when reviewing code (Part 2)
5 strategies to reduce friction when you review code.
Get the free AI Agent Building Blocks ebook when you subscribe.
Do you think you are dedicating your time to someone else when you are reviewing code?
You are not alone. I thought the same. But it’s your time. You are diving deep into the codebase. You are referencing software principles to improve concrete parts of the implementation.
It’s said that the teacher learns more than the students. I encourage you to think of Code Reviews (CRs) as time spent structuring your thoughts. This will reduce the resistance you feel against CRs.
We already went through a first round of CR mistakes and tips in 🚫 Avoid these mistakes reviewing code (Part 1). Today, we have 5 tips to make your reviews more efficient and reduce friction.
Write comments like a white paper: With references
The discussion goes nowhere when it’s about opinions. However, when I reference a single source of truth, it’s no longer about what you or I think.
When talking about styles, reference a style guide for the language. Usually, each company has some practices they follow. Keeping consistency with the surrounding code in the package takes precedence over any style guide.
When discussing software design, reference well-accepted principles: Design patterns, SOLID principles, or the AWS Well-Architected framework
This IAM Policy contains more permissions for SQS than the lambda needs. Can we limit it to <list of policy statements>? This way we follow the principle of least privilege <link>
Improve the CR 1 or 2 levels. Don’t aim for perfection
We can’t expect a new grad recently hired to write the same code quality as a senior engineer with 10 years in the company.
And that’s fine. We are all learning along the way. The CR process will help the new grad to learn faster than by just reading theory. If the code is not degrading the codebase, don’t hold your approval unreasonably.
In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect. - Google engineering practices
If the author brings a D-quality CR, aim to upgrade it to C-quality with your comments. Over time, the author will bring C-quality CRs as a default, and with your review, you both create a B-quality CR…
There is no perfect code, only better code.
Limit scope to lines modified
It’s tempting to ask to fix a couple of things as we are modifying that file in this same commit.
Stop yourself when asking about a line that is not modified in the diff of changes or when the modified lines don’t affect negatively those other lines.
If your comment was already applicable before this CR, then it’s out of the scope of the CR.
If it’s something to be done, create a new ticket with that purpose.
When starting doing little requests, the authors may give you their hand and you’ll end up taking their arm. Once you start the little changes, you’ll see little changes everywhere.
Know when to find an external opinion
Ideally, you and the author find agreement. The second best scenario is that someone concedes. There’s a full Amazon Leadership Principle about knowing when to concede (Have Backbone; Disagree and Commit)
But sometimes the discussion is going nowhere. It’s affecting both author and reviewer, wasting time and taking your peace of mind.
Don’t continue a thread of messages that is going nowhere. Schedule a meeting to discuss it, virtually or in person.
Acknowledge the good parts of the author’s proposal, and explain your concern and the tradeoff you think is favorable.
If you can’t find agreement even talking, it’s clear you are in a deadlock. You both should be happy to escalate to a third person who unblocks both.
Agree with that third opinion, even if it's not what you wanted. 2 people already took the same tradeoff, it’s time to disagree and commit. This doesn’t mean you change your mind, it means you don’t block progress.
And never, absolutely never, come back later saying “I told you so”. Even if you did, that’s not good in any relationship. Not in your personal life, not at work.
Remember that reviewing a CR is not about everyone doing as you say. It’s about having shared ownership of the codebase and creating better code than a single person could.