When conducting code reviews, the commentary provided can significantly impact both the code's quality and the developer's growth. Below, we will delve into the types of comments that can be made during a review, the rationale behind them, and how they can be structured to be most effective and constructive.
Courtesy and Clarity
It is essential to maintain a courteous and respectful tone while being clear and helpful. Comments should always focus on the code and not the developer. For example, rather than saying, "Why did you use threads here when there’s obviously no benefit to be gained from concurrency?", it would be more appropriate to comment, "The concurrency model here is adding complexity to the system without any actual performance benefit. Because there’s no performance benefit, it’s best for this code to be single-threaded instead of using multiple threads".
Explaining the 'Why'
It's not always necessary to include the reasoning behind every comment, but it can be particularly helpful when the reason behind a suggestion isn't immediately obvious. Explaining your reasoning can help the developer understand the comment's intent and how the suggestion improves the code's health. This practice also anticipates and reduces the need for follow-up questions, saving time for both the reviewer and the developer.
Providing Guidance
While the ultimate responsibility for fixing issues lies with the developer, striking a balance between highlighting problems and offering direct guidance can be beneficial. Pointing out issues and allowing the developer to decide on the solution can be a powerful learning experience. However, there are times when explicit guidance, suggestions, or even code snippets can lead to a better outcome and facilitate the review process.
Labeling Comment Severity
To make the intent of a review clear and help the author prioritize actions, consider labeling the severity of your comments. Here are a few labels that can be used:
Nit: This is a minor point that could improve the code but isn't crucial.
Optional: This is a suggestion that the developer might want to consider, but it's not mandatory.
FYI: This label is used for comments that are informational and don't necessarily require action in the current change list (CL).
These labels help ensure that the developer correctly interprets the comments and doesn't mistakenly assume all comments are equally critical.
Accepting Explanations
If you ask for an explanation and the developer's response clarifies the code sufficiently, it can be acceptable for the explanation to result in added code comments rather than a code rewrite. However, it's important to ensure that explanations are not just covering up overly complex code and that they add genuine clarity for future readers.
Handling Pushback
When a developer disagrees with a review comment, first consider their perspective. They may have insights that the reviewer hasn’t considered. If their argument makes sense from a code health perspective, acknowledge their point. If not, the reviewer should continue to advocate politely for the change, explaining why they believe their suggestion will improve the code health. It’s important to stay respectful and understand that insisting on improvements is part of maintaining high code quality, not a personal critique.
Incorporating these types of comments thoughtfully and systematically into the code review process can lead to more effective reviews and contribute to a culture of continuous improvement and learning within the development team.
