Why you should stop doing code reviews!
I would like to thank Baptiste Barbieri, Antoine Guy, Maxence Zajdlic and Rafael S. Ward for their time <3.
Code review is one of the most used methods in software development. We all know what are the benefits of doing code review.
To sum up, doing code review enables verification of new code quality by identifying future bugs, ensuring tested code, and ensuring readable code.
Moreover, by reading code written by others, developers can learn from others. A junior developer can learn by reading the code of a more experienced one. They can ask questions using comments. Vice versa a senior developer can check the code of a junior developer and provide constructive feedback.
I won’t go deeper into code review benefits in this article. For more information, you can read the amazing work done by Trisha Gee. For instance this blog post.
However, is code review really worth doing? What are the drawbacks we faced doing code review? I tried to list all code review antipatterns I faced and I’m sure you faced some of them. Several times.
Code review pitfalls
The “ping pong” review
Some comments are added, you take them into account, some new comments are added, you take them into account, new comments are added…and so on. It’s really frustrating for the merge request author, even more when it happens with several reviewers.
The “re-design” review
Once the merge request opens, you receive comments indicating a large design issue. It’s not a simple correction, but the whole implementation must be redone. This is a demoralizing situation for the author and the reviewers and a waste of time.
The “ghosting” review
Comments are added, taken into account and then you wait for the approval. The reviewer has disappeared and you have to wait before merging. Even if other reviewers have approved the merge request, you have to wait for the ghost reviewer.
The “waiting for” review
Your merge request is not the most attractive and you are still awaiting approval.
The “TL;DR” review
Changes are very important but reviewers didn’t read it before approving it because it was too long.
The presentation review
You have done a huge refactoring or the changes you bring are really complex to review and your team is asking you for a presentation or a mob review. Now you have to find a time where everyone is available to do the code review, it’s only the beginning of your merge request journey.
The “convention style” review
Some comments on the merge request are merely about coding style. For instance, there are too many break lines or spaces. Those kinds of reviews could be avoided with a linter.
The “too small” merge request
If one line of code has been modified, you tend to think that it will be approved and merged fast. BUT! Instead, it triggers a lot of discussions. We are typically in a bike-shed effect.
The “keep rebasing” review
Your merge request is waiting for approval, while other merge requests are merged and you have to keep your code up to date and potentially resolve conflicts.
The “inconsistent feedback” review
Sometimes you get a comment and don’t know how to handle it. Most of the time, the review expressed a feeling more than a clear solution. For example “This function seems too complicated for me”.
The “after merged” review
The merge request has been approved and merged. Great! But a new challenger has appeared! This challenger is perhaps knowledgeable about the project or on the technologies used and he wants to make some changes. Most of the time, a new merge request will be opened by this person without sharing it with the first developer.
The “conflict” review
Expressing or receiving feedback can be complicated, people can take it personally and it can lead to some tensions in the team.
The “elderly” merge request
The merge request is open for a long time, it could be a couple of weeks, a month or even a year. The merge request is so old that most of the developers don’t know if it’s still relevant to merge it so the merge request will just sit there.
The “not working” review
The CI pipeline passed, and the merge request has been approved but once merged and deployed… it doesn’t work! Let’s do a full cycle again.
The “Full-time job” review
Code review can be a full-time job! A developer in a team could spend their entire time doing code review. Furthermore, it’s not uncommon to have developers in a team who are spending way more time than others doing code reviews. Not everyone does code review in a balanced way and it can lead to struggles within the team.
The worst thing about all those patterns is that they can be accumulated. You can be in a ping pong review, suddenly land on with a ghost review, keep rebasing and finish by having some tensions in your team.
Stop doing code review?
So what are the solutions to avoid those situations? Most of these problems could be avoided by sharing early. Before what? For instance, sharing about coding guidelines, architecture, coding practices or design before jumping into the code. All of these methods and practices are living, moving with time and have to be updated during the project lifecycle.
It’s merely an Agile value: having feedback as soon as possible. Feedback coming from code review is too late.
In my experience, code review creates more problems than it solves. Code review is only good when developers work mostly in an asynchronous way and in different time zones. But in most cases, developers are working in the same team, on the same project at the same time and code review is only a way to avoid exchanges with colleagues. Nowadays, code review is a cargo cult.
Most of the time code review should be replaced by pair programming or better still ensemble programming (also known as mob programming). In that way, you have at least two people working on the same piece of code and you always have:
- knowledge sharing, both business and technical knowledge
- proof-reading
- developing cohesion and team spirit
Doing pair programming, doing synchronous development saves from ghosting, from ping pong comments, “TL;DR” review, “waiting for” review, “convention style” review….and so on. Actually, working together avoids all of the pitfalls of code review.
If you are developing using TDD, pair programming can even be more fun by doing ping pong pair programming.
Lastly, I won’t debate about the simplistic idea that having two developers working together on the same task is more expensive compared to having them working alone. The synchronisation process in all steps is one of the most expensive and code review is one of them.