I analyzed code review best practices for a year. This is what I learned.
Over the past year, I've been highly focused on one specific area: code reviews. I've analyzed and interviewed dozens of teams of varying sizes, industries, and countries to truly understand the best
Why?
Code reviews are important.
Many people think that code reviews are only for improving the code, guaranteeing that the requirements for different stakeholders are met, it may be security, maintainability, performance or only functionality, depending on the environment.
But code reviews is not only about improving the code, they are many times the best opportunity of improvements and new learnings for developers inside the team.
Also, recent research (DORA 2023 report) has confirmed that improving code reviews can speed up software delivery performance up to 50%. Take a look at my summary of the report here.
Code reviews should be FAST.
This is crucial. Typically, unless you use something like SHIP, SHOW & ASK (which I recommend), all code reviews are blocking by default.
This is, until the changes meet all the requirements and get the necessary approvals, the code is not going to production.
This is something super important: We should optimize for the speed at which a team can ship product together, not for the speed at which a developer can write code. In fact, this is the first phrase of Google’s engineering practices document.
This translates to replying to code review messages as fast as possible.
Code reviews shouldn’t break your FLOW.
Being in the flow state is that time when you are super focused on a task: it can be designing, programming, whatever you are doing. Some DevEx frameworks (such as the one presented in the paper: Devex: What Actually Drives Productivity) place the flow state as a top priority subject for developer productivity.
The devex triangle from the paper DevEx: What actually drives productivity?
Flow state is very important for the efficiency of the group, so if you are a developer and you are in flow state, try to keep yourself in that state as long as you can, avoid switching tasks and as long as you can, avoid interruptions.
Being in flow state is the only reason to pospone replying fast to code review messages.
I love this image I found on reddit about the dilema between flow state and interruptions.
Credits to nkukard.
Communication is IMPORTANT.
Normally code reviews are made in a written format. Humans communicate much worse in written format. Your corporal movements are not there. Your voice tone is not there. So misunderstandings are much more common.
This is why some new communication frameworks are raising such as conventional comments. Similar to conventional commits, but applied to code review comments. Where you explicitly specify the intention behind your comments. For example:
question (non-blocking): At this point, does it matter which thread has won?
Maybe to prevent a race condition we should keep looping until they've all won?
Where non-blocking means that this comment is not blocking the PR from being accepted.
Some other tips in this area are:
Ask questions instead of affirmations.
Be kind.
If you have more seniority, spend time on mentoring, this pays off exponentially.
If you not agree on a specific topic with a teammate:
Don’t take it personally.
Listen carefully to the other explanation.
Discuss it in person or in a call, where effective communication is easier.
PRs should be SMALL.
PRs/MRs should be as small as possible. Only one change at a time. Only one purpose at a time.
We’ve all seen the meme and know it’s true:
credits to twitter.com/iamdevloper
Large changes make async code reviews super slow if we want to keep quality, or super low quality if we want to keep speed.
So what can we do when tasks are huge and we need to implement many changes to accomplish it and we can’t divide the tasks into functional small subtasks?
Stacked pull requests. This is a great alternative. Basically it means dividing the task into reviewable (<400 locs) PRs. The downside of this approach is that since these PRs depend on each other, a change on the first branch requires updating the depending branches too.
This fact can make you lose some time synching the depending branches manually with ‘merge’ or ‘rebase’, this is why some tools where born such as git-town, ghstack or graphite.
Credits to Dr. McKayla
Pair programming. Some companies use pair programming as the default way of developing. This also means that the code is already being reviewed by 2 people while being developed, so you don’t need to do a conventional async code review afterwards.
Notes on pairing.
In the past, some recognized authors such as Dave Farley, author of Continuous Delivery, claimed a controversial idea (read comments on video or here) that code reviews through PRs are just a bad idea.
In this video, Dave proposes pair programming as the best code review solution for every case, for every type of company, every type of developer and every type of task.
I follow Dave, and I agree on most of the things he says but most of the time things are not just black or white, but somewhere in the middle depending on different factors. Pairing is one of those things.
As a developer, you need to know yourself in order to understand in which scenarios you feel more productive pairing, with which type of people and on which type of problems.
In my case, typically, I only feel productive while pairing in the following scenarios:
Mentoring a new person on the team.
Brainstorming creative solutions for a complex problem.
However, it’s very hard for me to achieve flow state while pairing, and as we discuss earlier in this post, flow state is a key factor on development productivity.
Final thoughts.
Code reviews need to happen fast, this is replying fast to code review messages.
Being in flow state must be the only reason to pospone code review messages.
Communication is important. Be nice. Listen. Discuss in person/call if necessary.
Only one purpose per PR. Small PRs. Stacked PRs if necessary.
Try pair programming with different people and tasks. Discover when pairing is useful for you (and your team).