đ 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.
đ€Ž #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.
đŻ Conclusion
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