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.
Originally published at https://vgallet.github.io on December 14, 2023.
Top comments (1)
Nice title, just clickbait-y enough to bring someone in but then holds to it argument. I think you basic thesis is more or less correct. Sadly pairing/mobbing can be quite difficult in many circumstances. Another thing to note is code review, or at least PR approvals are sometimes needed for regulatory purposes.
I wrote a bunch of words addressing a lot of the individual types of failure and where the break down comes from. But as you say the common patterns applies to most issues; Most of which can be solved with early feedback.
First type of issue: Failure of specification.
The task specification failed to properly explain what was needed. This manifests as the ping-pong, redesign, not working, and after merge types of failures. The implementer did not fully understand what they were supposed to do, the 'wrong' people were assigned and/or the 'right' people were not consulted.
The solution here is for the implementer to ask questions early in order to understand the work and for others who are 'area experts' to review the spec and approach. Having clear and documented implementation patterns also helps here.
Second type of issue: Failure of process.
The team's development process and tooling failed. There are two flavors of this the hard and soft. The soft failures are team process failures. These often manifest as the elderly, keep rebasing, TL;DR, waiting for and sometimes too small failures. In these types of failure the team's process does not allow for or emphasize the importance of code reviews. And more importantly team likely does not divide things up appropriately to avoid conflicts in the code and overly large amount of work.
Third type of issue: Failure of responsibility.
This really catches everything and is the most human and difficult part. A lot of the issues that manifest during issues with code review can be boiled down to the fact that most software developers think of themselves too much as coders. It's an easy trap to fall into to think that writing code is your job and everything else that you do is distracting and taking time away from that. There is so much more to being a developer/engineers than that.
Implementers must: talk to the PM and/or designer to make sure they understand the specs of a request. Additionally they should communicate back to the requester if there are any major technical issues that exist that could cause an issue with an implementation as specified. Implementers should also often communicate with other engineers to vet their intended implementation. Finally after implementation, usually in something like a PR, they must communicate to those reviewing the code if there are any ambiguities or particularly complex pieces.
Reviewers must: realize that reviewing code, vetting implementation ideas, and otherwise assisting their fellow engineers is part of their job too and not just a distraction from it.
Managers/Leads must: ensure that they create a team environment were the two above points are emphasized and respected. People should never be afraid to ask questions and feedback channels must be open and amicable at all levels.