Code Review: The Dark Side

Introduction

Code review is a fundamental aspect of modern software development, offering numerous benefits like improved code quality, knowledge sharing, and early detection of issues. However, when misused, code review can become a tool for obstruction, delay, and even personal agendas.

In this blog post, we'll explore some common code review antipatterns that can turn a collaborative process into a frustrating experience.

The Antipatterns

1. The Death of a Thousand Round Trips:
   • What it is: This occurs when a reviewer provides a series of minor, nitpicky comments, requiring multiple rounds of revisions before the code is approved.
   • Why it's bad: This can be extremely frustrating for the author, slowing down the development process and potentially discouraging them.
   • How to avoid it: Focus on major issues first and provide constructive feedback that helps the author improve their code.
2. The Ransom Note:
   • What it is: The reviewer holds a crucial patch hostage, demanding additional, unrelated work be completed before approval.
   • Why it's bad: This can be seen as manipulative and can hinder progress on important features.
   • How to avoid it: Keep reviews focused on the specific changes being proposed and avoid making unrelated demands.
3. The Double Team:
   • What it is: Multiple reviewers provide conflicting feedback on a single patch, creating confusion and delays.
   • Why it's bad: This can be frustrating for the author and can lead to unnecessary rework.
   • How to avoid it: If multiple reviewers are involved, ensure they coordinate their feedback and avoid contradictory comments.
4. The Guessing Game:
   • What it is: A reviewer provides vague, unhelpful criticism without offering specific suggestions for improvement.
   • Why it's bad: This can leave the author feeling lost and unsure how to proceed.
   • How to avoid it: Provide clear, actionable feedback that helps the author understand the issues and make necessary changes.
5. The Priority Inversion:
   • What it is: A reviewer introduces a major issue late in the review process, requiring significant rework after minor changes have already been made.
   • Why it's bad: This can be demoralizing for the author and can lead to wasted effort.
   • How to avoid it: Identify major issues early in the review process and provide feedback accordingly.
6. The Late-Breaking Design Review:
   • What it is: A reviewer demands changes to the overall design of a large piece of code, even if the changes are already in progress.
   • Why it's bad: This can disrupt the development process and lead to unnecessary rework.
   • How to avoid it: If you have concerns about the overall design, raise them early in the development process.
7. The Catch-22:
   • What it is: A reviewer demands changes that are contradictory or impossible to satisfy.
   • Why it's bad: This can be frustrating and demoralizing for the author.
   • How to avoid it: Provide feedback that is clear, consistent, and achievable.

Conclusion

Code review is a valuable tool for improving code quality and collaboration. However, it's essential to use it responsibly and avoid the negative behaviors described in this blog post. By following best practices and fostering a positive review culture, you can ensure that code review remains a productive and enjoyable experience for everyone involved.

Reference: Code Review Antipatterns, Simon Tatham, 2024-08-21 here.