How to make the best of Code Review

In our development team, we do code review on every single merge request (aka a pull request for GitHub users) without exception. Every MR is reviewed by two developers of the team. I personally love it as I think it has a lot of advantages (as I’ll explain below). But code review can also quickly become a bottleneck in software development.

At some point we felt like some of us were spending a lot of time on code reviews, leaving pretty little time to actually produce code, so we came with the idea that maybe we needed a way to better distribute the code reviews so that the workload would be more even (that is, trying to find a way everybody would do the same “amount” of code review).

While looking for the best way to qualify and quantify a “difficult” code review, I found a ton of articles and discussions about code reviews, from ways to make it less painful to bots that distribute them automatically.

Why you should embrace code reviews

I’m pretty maniac about clean code and I love giving and getting feedback (which I consider as part of mentoring, which I find extremely important), so I was amazed during my research to learn that most developers don’t like code reviews (yeah, le me can be a bit naïve sometimes).

Code review has a lot of advantages for everyone:

For developers (authors of the code being reviewed)

  • Probably the best and most efficient way to improve your code skills after you went through the basics.
  • Great way to know which topics you should focus learning.
  • Learning opportunity: we cannot know everything, the code reviewer may come with an idea on how to implement a new functionality, using a tool or an API we didn’t know about yet.

For code reviewers

  • Opportunity to develop your mentoring skills.
  • Learning opportunity: yeah, the reviewer can also learn and discover new ideas from the code they read (if it doesn’t happen, it might be time to change the code review process in your team).
  • Know what’s going on in the project, what new features were implemented…
  • A nice way to notice your evolution, to notice that you can teach or explain things to others that you weren’t aware of or confident about some time ago.
  • Can be an ego booster: in a job where it’s really easy to feel bad some days, it does feel good to participate in others’ evolution and be able to teach them some things.

For the team

  • Maintaining a clean code base and enforcing good practices, which will ease coding for everyone in the team but also onboarding new team members.
  • Team cohesion, discussion starter, collaboration, healthy debates.
  • Ensure the diverse visions of the team are heard and benefited from: some will have an eye for functional problems, some for technical issues, some for UX, some for reusability and app architecture…

How to make code reviews less painful

Code reviews shouldn’t be a burden. If a lot of developers don’t like it (apart from those who don’t like it because their ego can’t stand it…), it might be because the processes are not on point. Here are a few ideas for easier code reviews:

Make code reviews short

I found a few sources that stated that a code to review should be less than 400 or 500 LOC (lines of code) long. A code review should also take less than one hour to complete. Longer than this, the reviewer won’t be able to focus enough and will experience a review fatigue. So, pure waste of time.

A first step is then to create tickets with the right scope. Anyway, apart from code review issues, it’s never good to have a developer work on a ticket for too long (hello merge conflicts and rebase nightmares).

It’s also every developer’s responsibility to split their ticket if they appear to be bigger than expected. Finally, some automated solutions exist to prevent developers from opening merge requests that are too big (like a bot that’s integrated in your CI pipeline and will warn you if your MR is too long).

Automate every uninteresting aspect of the code review

Everything that can be handled automatically shouldn’t be the reviewers’ task. Code style and conventions relative to the programming language can be automatically checked. Every developer in the team should work with a common linter locally, and the code style should be checked again automatically in the CI pipeline. It’d avoid the pain of the reviewer to search for missing white spaces or semicolons, and the pain for the developer to have to push changes for adding a white space or a semicolon.

Code inspection can go further (code repetition, security issues, errors not caught properly…), with tools like SonarQube.

Decide together what’s a code review

The team members should agree on what’s a code review, what’s expected from the reviewer and what’s not, get a clear vision on the how-to (like a checklist). I learned recently about the concept of “Social contract” or “Team contract” via the Open Practice Library and I think it’s a perfect use case.

Code review expectations and process should be agreed upon and documented on the team wiki after having been discussed together. It should be an evolving process: the needs would change if you integrated an automated tool for spell check, code style, and so on, or if something is not working well.

It’s also important to emphasize on the why: people understanding why they have to execute a task (what’s in it for them, for the team…) are generally more incline to do it and do it well.

At Google, they defined three gates through which a code review has to go. They represent the things they are expecting from the code review process.

Be a team-player, ease the reviewer’s job

If you’re the author of the code, just don’t be a jerk. Team spirit is really important in code review processes. If you push code without considering the job of the code reviewer, you’re just making it a pain for them… which will lead to the corrections being a pain for you in the end anyway.

It happens to every developer to push code with errors, that’s basically one of the reasons we need code review. But it’s not an excuse for delivering dirty code and not paying attention at all, solely relying on the reviewer to improve it or make sure it works. You should review your own code before opening a merge request, the same way you’d review someone else’s code.

How to choose the best code reviewer for your merge request

I spent a lot of time on this topic as it was our main concern in the team. And the question is not easy to answer. There is definitely no one-size-fits-all solution, but here’s some food for thoughts.

Should everybody share the same load of code review?

This is again a question worth bringing up while establishing the team contract. It really depends on your goal: if you just want code reviews to be done quickly without any other concern, code reviews would most probably be distributed unevenly, and fast reviewers would do more review.

This raises several problems: first, “fast” doesn’t mean “fast and efficient”. You also may be preventing really good developers from participating in the code base as much as the others. Then, you wouldn’t take advantage of the diversity of the team, both in authoring code and reviewing it. And of course, those who are not happy about doing reviews will just go slower and slower so they can make less and less of them.

So yes, to a certain extent, distributing the workload evenly seems important to me. Even though perfect balance is out of reach, it’s important to make it a team effort. But then comes the crucial question…

What defines the workload for a code review?

In other words, how can we define that a code review is more time consuming than another, because balancing the workload doesn’t only mean giving the same number of MRs to everyone, but also taking complexity into account.

To calculate the weight of a Merge Request, we can rely on several parameters: the number of lines of code, the number of modified files, the type of change, number of existing comments, whether a first review was already done, for how long it’s been open…

The easiest criterion is probably the number of lines that were added or changed. If we make it a rule to limit the LOC (as mentioned above in the article), it means there is a smaller difference between MRs, and weight is less of a concern.

Should you auto-assign code reviewers?

As far as I could find information, code review auto-assignment tools currently on the market are only capable of distributing tasks randomly taking into account tags associated to coworkers (so, not at all taking into account the difficulty of the MR or the work already done by that review in the current work period, for example).

The team members would generally be tagged with their field of expertise so that, for example, a merge request for a front-end development would only be distributed randomly among front-end developers (you can tag the members as you wish, by project, programming language, tier, seniority, whatever).

The other solution is a plain random among all team members. This allows to skip the time and effort of choosing the reviewer(s). It also allows to distribute the different types of code reviews more evenly.

When you manually assign a code review, you could choose someone specific because you know they’ll make great comments and you’ll learn from them, because they’re particularly good at this technology/language (or at the contrary, you could be tempted to choose someone less picky if you want your ticket to be validated fast).

This means you’d often go for the same reviewer(s), probably senior developers, not allowing juniors to evolve in code-reviewing, or people less at ease in some fields to challenge themselves. You’d also not benefit from the diversity of points of view and experiences.

Can you quit code-reviewing if you’re pair programming?

Another interesting point of view I came across is that some development teams decided to “replace” code reviews by pair programming. For example, a “one author – two reviewers” workflow would become a “two authors – one reviewer” workflow. In the end, there were still 3 pair of eyes on your code, but the dynamic is different.

I’ve not done a lot of pair programming in my life. I’m pretty sure I wouldn’t want to do it every time I develop, but more occasionally to get knowledge on a particular topic. The advantage of this approach is that big problems could be found earlier, while code review may raise a big issue when the author thinks they already made most of the work. On the other hand, having to rewrite your code like this might be a very efficient way to remember not to make the same mistake in the future.

Let’s not forget that pair programming is less challenging than reviewing some code you’ve not worked on at all. When pair programming, one of the developers will probably take the lead (even if unconscious), and the second one will be influenced by the direction taken (even if unconscious).

When you do a code review, you have to discover it all from the start, which can raise different questions and ideas without being influenced by the other’s vision.

Any thoughts on code reviews or what you just read?
I’d be glad to discuss in the comments section!

Leave a Reply

Your email address will not be published. Required fields are marked *

Skip to content