Let's admit it.
Code reviews are bottlenecks.
Most code reviewers are gatekeepers.
By controlling and ultimately deciding what gets into the main branch they slow down the entire process - screwing the original estimates and jeopardizing the sprint.
I am myself often a gatekeeper.
Sometime ago a colleague said jokingly that I am like the Dungeon Boss in videogames: the developer has to go past the level - the implementation of the feature described in their ticket - and when they are done, or they think they have accomplished their challenge, here comes the hardest part, the end boss: the code reviewer!
Once, a ticket that was estimated for 1 day, (but took 3 days of development), required almost 4 hours of code review with endless threads, and then almost 2 days to apply all the changes that satisfied the reviewer.
What did go wrong?
- naive and optimistic estimation
- unclear or only partially documented task definition
- lack of code design
- a desire of be quick
- misunderstanding of the scope of the task
- unclear definition of done
- first experiences with working remotely due to the lockdown and the pandemic
- communication issues
- a sloppy implementation
- a nitpicking reviewer
- some rivalry among the envolved parties
This was clearly an exception, and it would be too easy to only blame the developer, or the reviewer - the reasons and causes of this time consuming bloodbath were many and reveal some disfunction in the process.
Should we stop doing code reviews?
A lot of developers don't like them when they have to review, because it is boring or they feel bad at judging other people's code
Many developers don't like them when they are the ones being reviewed, because, well, nobody likes having its mistakes and flaws being pointed out.
Project Managers and Team Lead don't like them because they are the biggest incognita in the success of the sprint, often deadlines are not met because of the changes requested by the reviewer, and because to to the PR threads and endless back and forth and comments the mood in the team can become toxic.
But... Are these good reasons to stop doing code reviews?
What is the benefit of having them? How can we make them better?
Spend time on your Code review
Do not just carelessly scroll a PullRequest ( or MergeRequest if you are using Gitlab), as a daily annoying chore.
You are doing something very useful!
- for yourself: you can learn something about the new feature being implemented or about a bug being fixed, about coding styles and about different ways of approaching a problem.
- for the reviewee: you can share some knowledge and give advice and help people grow and become better developers.
- for the team: you contribute in raising awareness of the codebase and adoption of coding standards to make the team a "well oiled machine"
- for the project: you help keeping the quality high, prevent bugs or memory leaks to sneak into production, contribute in delivering a great product that is maintainable and extendable.
Therefore, take some time to understand the task, and understand the changes, don't just check if the snippet of code in the changes is just not wrong or ugly.
And take the time to properly write useful, reasonable and respectful comments.
Automate whatever is possible
Code review should not focus on gritty details as indentation, brackets, semicolumns, or any other coding standards that a good linter could detect.
Pick any linter, we use a very opinionated one ( and we love it, and somehow hate it when new stricter rules are introduced) but it helps us to avoid wasting time on such things. Just run the linter before every commit and you can focus on the interesting stuff - the code implementation!
Be clear, be supportive, be kind
In the same way you are not carelessly scrolling a PR you should not be there staring at the screen with an evil smile looking for a flaw so that you can point it out bragging about your experience, your skills, how good you are.
You are there to help - and mentor - the reviewee, to avoid that wrong solutions or bugged implementations make it into production, to help the team deliver better code, to avoid that bad code gets into the code base and degrades the quality, and the future developer experience.
Reduce conflicts
Some times code reviews are a problem simply because they surface rivalry or power-games in the team.
Maybe there are 2 senior engineers engaging in an alpha male battle for the next promotion and raise.
Maybe a (brilliant and skilled) junior developer is resentful for their lower title and wage and and is trying to call out other regulars/seniors.
Maybe it is a micromanaging and controlling leader that simply wants to state his authority over team members.
Again this is not a problem of Code Reviews, but often, to avoid these conflicts, the solution is to avoid having Code Reviews - which is bad for the quality code base and for the growth of the team.
If code code reviews become long, tedious and ultimately conflictual, try pair programming.
I can't guarantee it wont be conflictual, power-games will arise in pair programming too, but with different nuances, and the process is overall sped up.
Synchronous communication is better and more efficient.
A poorly written comment can be misunderstood, both from technical and emotional point of view.
Then the reader asks for clarification, the thread increase in size with both people trying to make themselves clearer but the result is often frustration, anger, passive-aggressive behaviour or condescending language.
While talking and seeing each other, sharing a screen and pointing directly at the code, we have more context and need less time to recalibrate a misunderstanding or ask for clarification.
On top of that, by pair programming you prevent the frustration (or the rejection) that comes from a comment that requires the developer to throw away the code they wrote.
Things are discussed on the go, before they are being written, before you invested time on them and the developer become less attached to their solution. (check also my previous post: Keep your Ego away from coding)
Keep your Pull Request small
One of the reason why Code review become too time consuming, boring and ultimately, useless, is that too often Merge Requests are too big! There is simply, way too much code to review, too much context too understand which would require too much time to review - time which is often not even clearly allocated in the team sprint.
Code reviews should be done within minutes.
In order for this to be possible, another great suggestion from Cory House:
Focus on 1 thing.
Only change what’s necessary for that 1 thing.
Handle bugs, new features, and refactors in separate PRs.
When I see an unrelated improvement opportunity, I handle it in a separate PR.
Be proactive and clear in your communication
This is true for reviewer and reviewee. Spend time in writing your comments, the goal is improving the code base and merge the feature into main. therefore the exchange of messages should be effective.
Imagine this comment about an edited script in package.json - apparently unrelated to the feature which is focus of the merge request:
why is this change necessary? - was CI broken?
To that the reviewee, replied:
fixed. (while actually reverting entirely the change)
How helpful is that answer?
Questions asked during code reviews have multiple reasons, we might have not understood the issue, we might need more context to understand the change.
Replying FIXED does not help understand why that change was necessary in that MR or needed at all.
Of course, communication works in 2 directions.
So let's imagine my comment from why is this change necessary? - was CI broken? to something less judgemental or invasive:
I noticed you added a change to package json which is not part of the feature.
Is this a change necessary for your local tests or did you find that something in our CI/deployment was broken? (if so that is a great finding! Thanks for the fix ! -
otherwise please remember to revert your local changes that might break our CI pipeline. :wink!
oh, maybe next time create a mini MR directly to main so that everyone benefits of the same fix immediately.
Now let's imagine what the reviewee could have answered instead of fixed
honestly I did not quite get why the dependency we are using stopped working, but after some investigation with awesome_colleague we realised that
some technical explanation. As you pointed out though I realized that this change is only necessary while testing locally therefore I reverted my change - I will eventually create an additional script for local env and push it to a different branch/PR.
As you see, being concise while providing all the information, and without becoming too blunt is not that simple.
I am quite known for being pretty direct, but if I write "please rename this method XYZ", (especially if we have been working together for months or years) you can be pretty sure there is no underlying offensive message or personal judgement.
Or do i still have to say:
This method could be renamed as XYZ because that would make it both more concise and descriptive. What is your opinion about that?
(feel free to disagree with me in the comments - I am genuinely looking forward hearing different opinions and improve my communication)
The point is Both parties should do their best to avoid misunderstandings, and to assume good will on the other.
Use conventional comments
To achieve both conciseness and politeness a great tip is using Conventional Comments.
We have been using Conventional Commits for years now - they are very useful to have standards within the team, make the commit history nice, clean and readable, and the Changelog is one of the main pros from adopting them.
Thanks to this post from NickTaylor I found out about
Conventional Comments and immediately fell in love with them.
They help to smooth out comments that could be misunderstood, to be polite even when you really want to save up keystrokes.
And also provide some more context about the action necessary - should the reviewee work on every single thing pointed out, immediately as a priority, or can the code be merged and then "refactored", are those just suggestions for the future or mandatory steps?
A comment saying simply This is not worded properly is a bit too blunt and also does not give clear instruction about what is necessary.
compare that to :
Recap
Most of the time when Code review become a bottleneck there are multiple reasons which we should address for the sake of the project.
Dismissing code reviews is not a solution - it would be like stopping writing code because there are bugs...
Code Reviews are opportunities to share knowledge, raise interesting questions, leveraging team skills and increase overall awareness of the codebase.
And they prevent buggy code to make it in production.
Discuss, agree and define standards within your team (linting, conventional comments, process of review)
Define guidelines for when things get rough: If a comment is unclear, instead of replying with a passive aggressive answer, ask in person or join a slack huddle, to sort out the issue quickly, don't let it escalate or go on for hours with multiple back and forth ( which distract everyone from what they are doing)
Don't have just one person become the bottleneck. Reviews should be shared among the team, be done by different ( and sometimes by multiple) people in the team.
Be kind.
Photo by Sajjad Ahmadi on Unsplash
Top comments (1)
Thanks for posting! This is good advice.
Another way to be proactive is to architect the solution before you code with a stakeholder. Write up a doc with a proposal, talk about the solution with the approvers, update the ticket. Giving people a heads up goes a long way to preventing out of context replies or strange control issues and may prevent having to redo the work. Putting the doc in a knowledge base that isn’t a pull request would be preferable I would think.
As a reviewer, I’m a bit of a perfectionist but have given up being nitpicky. I wish others would do the same. It’s extremely annoying when code reviews are subjective. I try to be as objective as possible.
Most importantly, disarm everyone with kindness!