Thoughts on Code Reviews π
Why so many teams mess them up, and a modest proposal to do them better.
One of the major concerns I have about running Refactoring full-time is to lose some hands-on engineering experience.
I try to compensate by doing a ton of research and talking with as many people as I can, but there is no deny that I now have some detachment from things that, up until a couple of years ago, I used to do every day.
While this is mostly a source of concern, over time I have also found an unexpected flip side.
As long as I was deep in the weeds, I used not to question many engineering practices that seemed standard to me. After all, a startup has its good share of risks already, so why spend a precious innovation token on process? Do not reinvent the wheel, Luca.
The problem with this (perfectly good) advice is that, from time to time, the proverbial wheel needs to be reinvented. Conditions change, tooling gets better, and we eventually find better ways to work.
So, I have found that by being removed from the daily engineering struggle, I feel less attached to the way I used to work, which in turn makes it easier to question things here and there.
Lately, one of my small obsessions is how we do code reviews.
Code reviews look, to me, like an imperfect solution to two supremely important problems:
π Keeping code quality high β which leads to ease of change, fewer defects, less maintenance, you name it.
π Sharing knowledge across the team β which creates alignment, growth, resilience, flexibility, and more.
These are also self-reinforcing goals: shared knowledge helps keep quality high, and, in turn, high quality code is generally crisper and easier to understand for others.
Code reviews address these goals by making code changes inspected by multiple engineers β at least two: the author, and one+ reviewers. This, to me, not only looks fine: it looks like the only possible way.
Still, even if we agree on the above, there is plenty left to figure out, like:
When should reviews be made?
Should all code be reviewed?
What goes into a review?
Who should do reviews?
Today, the most common workflow for code reviews is async + blocking + mandatory, that is:
πΒ Async β a PR is opened, one reviewer (or more) is assigned, and they will perform their review asap.
βΒ Blocking β code isnβt merged and deployed until it passes the review.
πΒ Mandatory β you do the same workflow for all changes.
I believe this is far from ideal, so I spent the last couple of weeks writing down my thoughts about it, and how I think we can do better.
I also spoke with many people in the Refactoring community, which was supremely helpful in getting real-world stories, some of which are quoted throughout the article.
So, this piece is a conversation on the upsides and downsides of code reviews, starting from first principles, and discussing both familiar and unconventional workflows.
Here is the agenda:
πΒ Why code reviews matter β and why your innovation token is probably well spent here.
πΒ The scope of code reviews β what should go into a code review?
π Β Automate / Defer / Pair β my modest proposal for a healthy review process.
π½Β Choosing for your team β how your team's maturity and seniority affects reviews.
Letβs dive in!
πΒ Why code reviews matter
This question looks banal.
However, while the main goals (quality, knowledge sharing) may be obvious, I have found that reviews have second and third-order effects that are trickier to figure out, and worth discussing.
Code reviews are your main (and possibly only) feedback loop on how your team writes code.
This feedback loop not only intercepts defects: it aligns people on practices and culture. From high level engineering principles, down to naming conventions, chances are many of these are not only enforced byβthey are literally born out ofβcode reviews.
So letβs start with what happens when this feedback loop is not in place.
A no-reviews story β
I have worked once as an EM on a team that didnβt review code. When I joined, it was made up of 4 senior engineers: they were all extremely fast and experienced, and they just pushed code to prod all the time.
Things apparently worked, and code quality was good. But there were other, invisible problems:
πͺ Gatekeeping β over time, each engineer had developed their personal areas of ownership, which had become impenetrable to everyone else.
π Inconsistency β different parts of the code had completely inconsistent choices about naming, libraries used, folder structure, and more.
π No docs β since the general expectation was that everyone worked on their own, there was little incentive to write good docs and keep them updated.
Down the line, these problems led to more problems:
Collaboration β any conversation about design, tradeoffs, or estimates, was extremely hard, because everyoneβs ownership was completely siloed.
Hiring β onboarding new engineers, either junior or senior, was a nightmare.
Resource allocation β if we wanted to invest more in a specific product area, it was hard because we couldnβt put more people on it at will.
Key man risk β individual engineers got an outsized importance for the business and, even ifβto their creditβno one ever tried to take advantage of it, it was concerning to me as a manager.
I will stop here and wonβt bother you with how we tried to improve things. This story was to show that the impact of no reviews goes way beyond the βmore bugs in prodβ. In the long run it is detrimental to almost everything you do.
The other extreme β slow reviews π
Now, there is another end of this spectrum which is equally bad. Bad code reviews are usually bad in two ways, often at the same time: