How High Performing Teams Review Code
Busy being busy. Are they worth it?
Today’s Crafting Tech Teams issue is all about code reviews and pull requests. But before we start, some announcements for tomorrow’s stream. We’ll be looking at Dependency Injection in isolation by reviewing a Fireship video, afterwards we’ll be looking at Milan Jovanović’s new course of Pragmatic Clean Architecture. I’ll be handing out discount codes to live viewers. Link is at the end of this article.
By reading today’s issue you will learn the following:
The purpose of code reviews. How it all started. What happens in the absence of code reviews as a process?
“Standard” code reviews. What you think code reviews are supposed to represent and how it should be done is actually not the best way to do them.
High performance code reviews. Case studies from top companies and coaches about what works and what doesn’t adjusted for 2023.
An alternative perspective. Going forward, code reviews can be viewed not only as a merge-step, but as an entirely separate, lighter process.
💬 Join the discussion - Injection & Clean Architecture Stream on Thursday. Got questions? Want to see me react to Dependency Injection videos? Are you on a team using Clean Architecture and are wondering how to best inject frameworks and ORM concerns?
The purpose of code reviews
A product engineering process that has no form of code reviews will inevitably suffer from the soft skill factors of their team: human nature. When engineers are under pressure or lack motivation, they gravitate towards a certain set of biases and mistakes:
I’m confident it works, I don’t need tests
I covered everything
The shortcuts I took are necessary to make the deadline
The short-term gains outweigh the longterm issues
It’s safe to skip this
I can’t possibly test everything, no one does
And lo and behold, the project is behind and a chaotic mess. The purpose of code reviews—in any form— is to ensure that every change to the codebase passes at least two brains before making its way into the ether. The hope here is that the person not immediately involved will be calmer and more objective compared to any immediate pressures the author might be facing.
This is of course just a hope. And hope is not a very good tactic. Often, mandated code reviews without a clear outcome will keep a team just busy enough. This synthetic business slows them down so the negative impact of sloppiness and negligence can be micromanaged after-the-fact by more senior team members. If you want to learn more about how to write benevolent code reviews, I suggest the following previous issue. Afterwards let’s look at the process itself.
“Standard” code reviews
Pull requests. Block merge to main/master/trunk. Need X approvals. This is your standard process.
It works for your average team, but it has some drawbacks:
There are no expectations on how long someone has to wait for a PR to be approved
There are no incentives to keep PRs large or small
It’s unclear when in the project timeline and roadmap the PRs are happening. Do we interrupt each other? Wait until tomorrow?
Code reviews are great for knowledge sharing and bug hunting. However, the process itself requires discipline and an understanding of overall team dynamics, not just your individual skill.
When coaching teams on improving their reviews and cycle time, one topic constantly stands out:
They act shy—they don’t want to interrupt someone else to ask for a review. This leads to PRs waiting for several hours or several days!
Treating reviews of pull requests as a slow once-per-day ritual gives an illusion of best practice that undermines the team’s ability to move faster. The strict nature of "standard" code reviews creates an environment where engineers are more concerned with adhering to checklists than with creativity and innovation.
Fast feedback loops—minutes, no hours—are a key predictor to developer productivity (as per SPACE and DORA metrics). A developer’s contribution to the codebase has two major sources of feedback:
The automated CI/CD pipeline, including test feedback
Code review
That’s it. If you get these two parallelised and under 10 minutes, you’re in the top 1% of engineering teams and you can happily consider yourself a 10x team.
High performance code reviews
I love the dev.to podcast with LinearB:
Key highlights:
This one is very unintuitive: Interrupt the developer! You want Code Reviews done so fast that the original author doesn’t do a context switch and starts work on something else. This can spiral out of control where 2 devs are working on 4 things at the same time, waiting for a natural gap—which is usually lunch or end of day. This waiting creates a synthetic waiting loop of several hours that can be a few minutes if you simply interrupt the developer for a review. But only if…
PRs are kept super short. 400 lines is the maximum. It should take minimal effort to gain context for “What the heck am I reviewing?”
Don’t mix your intents. Keep a single target. If you notice something else is broken while you are working, do a separate refactor or bugfix first and then return to your task. Try to think about how to solve problems multiple problems with multiple PRs. Use your directory hierarchies to spot anti-patterns that lead to large, 10+ file PRs. Avoid shotgun surgery.
Designate pairs when you are not pair programming: Know in advance who will review your code and stay in close communication.
Andrea L. happened to post on this today with a super productive cheat sheet:
Give him a follow!
An alternative perspective
You can review code without doing code reviews:
Ensemble/mob programming
Pair programming
Automate certain aspects of reviews
A combination of all three is best. Stop wasting time reviewing style, major code smells or issues a linter can detect. At the same time, try to aggressively merge micro-PRs that have a few lines of documentation or config code changes. Make exceptions to your pipeline and auto-merge PRs where the pipeline gives you high confidence of not requiring review:
Refactoring-only changes that are test covered
Commits with several authored—proof that it was reviewed already by virtue of pair programming
Details that have no significance on learning or functionalities (e.g. semantic commits containing CI, typo or chore)
💬 Join the discussion - Injection & Clean Architecture Stream tomorrow
Mark your calendar! I stream every Thursday for an hour at 16:30 CEST. August 10th will be about Dependency Injection with Clean Architecture. We are reviewing a fireship video and a bleeding edge course about Pragmatic Clean Architecture (in C#!) released this week that goes like butter. Link bellow, see you there!
