📜 Apply these strategies when reviewing code (Part 2)
5 strategies to reduce friction when you review code.
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.
📑 #1 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>
📈 #2 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.
🗺️ #3 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.
👉 Enter your email to never miss the weekly post
🤴 #4 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.
🛋️ #5 Take your time and take breaks
My worst time reviewing a CR came when we were in a rush to deploy code before a deployment window closed for a big event in Retail (like Prime Days and Black Fridays). If we didn’t deploy fast, our code wouldn’t reach production in time. It was now or never.
We write code behind feature toggles, both to prevent bugs and for A/B testing. The new code wouldn’t be executed until clicking in a UI.
The CR came at 6:15 p.m. on a Friday. I had zero mental energy, but we needed the code to deploy through the pipeline during the weekend. We had to merge
I remember I was not checking the new code that would be executed when enabling the feature toggle. If that code breaks, we can just disable it and that’s it. Neither was I checking code quality or any good practices.
I only reviewed it to make sure the way we introduced the code wouldn’t break production the moment it got deployed, and then approved.
This code introduced a restriction in a country because of special regulations. Later we had trouble enabling in that country future iterations that had no legal restrictions because of the way we introduced it the first time.
You can’t do your best review at the last minute of the day, reviewing in a rush. But at the same time, you are working for a company and not for the code. Time to market is critical.
These scenarios made me reframe my approach: Try to review as soon as possible the code. It’s not to help the author but to help yourself do a good code review without being pressured by time.
You need time to review not only the diff but the surrounding code. I have spent more than an hour on some commits, and that’s fine.
But also remember to take breaks. I have found myself mindlessly navigating code files without understanding. It’s time to take a break, go for a glass of water, walk around, or talk to someone.
Your purpose with CRs is not to spend a certain amount of hours, big or small, but to be confident in what you have reviewed.
You may have read this wishing your team followed these practices when reviewing your code. We all think so, it’s human nature.
Don’t try to change anyone. Apply this and others will follow. Lead by example.
Think about it: What situation do you wish you had done better reviewing code? I’d love to hear about it.
Pick a single tip and start applying it. Just one, if we try to do everything, we end up doing nothing.
Thanks for reading another post. See you next week!
👏 Weekly applause
Small ideas with deep meaning:
Earn your paycheck before lunch → Do your deep work first, then you can allow meetings and distractions. Read in the newsletter A Life Engineered
Keep documented what your productivity system is like → Like a design document of your software architecture, keep up-to-date documentation. This ensures you are intentional and not just “adding things”. Listened in Cal Newport’s podcast
Define your personal success system → Some of us work better with small progress over time, like a marathon. Others with an intense sprint. Both are good. Do what works for you. Read in Ali Abdaal’s newsletter