Are you a software developer? Chances are you’ve participated in a code review, a process where one developer, having completed a piece of work, submits the work to his/her peers for scrutiny. The advantages of code reviews are clear: team knowledge sharing, building like approaches to problem solving, and occasionally, finding bugs before they make their way to production.

Disadvantages? These are a bit more subtle, but should not be overlooked. Egos are bruised when work is critiqued. Developers are not known for their ability to sugar-coat, and given the potentially impersonal nature of review, code comments can be misconstrued, perhaps delivered with a bit too much honesty, and worst of all, downright nastiness. Developer ego, combined with detached critique, and misinterpretation, leads to explosive situations, with a good chance of hurt feelings. Even when suggestions are delivered with best intentions, sensitive code owners tend to take shelter under a common set of excuses.

Here are some common excuses given during reviews by code owners, what you should consider when you hear them, and what the comments really mean.

1. “It’s not MY code!”

Maybe the worst excuse of the bunch? Often spotted after a reviewer comments on a piece of code that is particularly troublesome. An innocent question, such as “Why does this function return null?”, followed by “No idea, not my code!” This is especially bad when its occurring within the same team.

Why is it a bad excuse?

It’s a terrible excuse! All team members should own the team’s code. If the code under review isn’t up to standard, fix it. The purpose of reviews is not to blame or give negative feedback. The goal is to work together and find an agreed best solution for the problem. If the code doesn’t meet the agreed upon standard, or doesn’t solve the problem optimally, work with your team members – don’t pass the buck.

2. “That section isn’t under review!”

A personal favorite. A developer submits some pieces of code for review. A dutiful reviewer runs through the change list, and showing initiative, comments on fixes/ideas in other areas of the code apart from the changelist. When these comments are read by the owner, the owner responds, “That stuff isn’t part of the review! Stick with what was changed!”. The reviewer accepts the excuse, removes the comments, and life goes on.

Why is it a bad excuse?

I acknowledge there is a place and time for all comments. It can be frustrating as a code owner to see the reviewers “missing the point”, and commenting all over the place, making suggestions that are both not in scope, and untimely. Still, this is not a good reason to dismiss the critique. The best owners accept the comments, prioritize them appropriately, and ensure that the reviewers are focused on the changelist, and the work description driving the changelist. If reviewers sign off on the changelist, as well as comment around other areas of the code, you must accept these critiques professionally.

3. “That section isn’t finished yet!”

You receive a request to review code, and to your amazement, it is littered with #TODO, unfinished unit tests, and stubs. Clearly this code isn’t ready for production. Why has it been submitted? As a reviewer, you responsibly comment on these issues, only to receive responses of the sort “I haven’t done that yet”, or “That’s a work in progress”. You regrettably ignore these sections, trusting that one day they may be completed.

Why is it a bad excuse?

This is not an attack on stubs, and the unfinished. This is an attack against code that is clearly not modular enough to be submitted with unfinished pieces. If your process dictates code reviews happen as a last step before moving towards qualification, having unfinished pieces is potentially a sign of a poorly organized implementation. Unless it is made very clear by the code owner, all code submitted for should be in a finished state.

4. “That doesn’t matter, this is a bug/patch fix!”

This comment typically presents itself during a small code changed, designed for minimal impact. Think of a production patch, or a high priority bug fix. Clearly, the goal of the review is to identity the validity of the fix itself, ensuring the fix introduces no side effects, undesired behaviour, or security/performance issues. A reviewer may, having identified none of these issues, comment on other aspects of the code, for example, a design issue, readability, or expressiveness. The code owner responds “That stuff doesn’t matter here, this review is only for the bug fix”. Is this a valid point?

Why is it a bad excuse?

It can be, although I would say most often no. I have seen situations where developers do not pay enough attention to the purpose of the review, especially in the case of a high priority bug fix. Given a certain skillset, it is natural for developers to point out issues where they are strong, e.g. a performance expert will certainly notice performance issues with great ease, and tend to comment on them. Does the purpose of the review negate the importance of these comments? I say no, with the caveat that the developers are also responsibly signing off on the original intent of the review. The code owner should ensure that the reviewers are happy with the fix, while also receiving additional comments, and deciding any further future action to take.

Code reviews are important. They spread knowledge within a team, cultivate a team’s approach to solving similar problems, and help the less experienced gain insight. The also serve the purpose of occasionally finding bugs while they are relatively cheap to fix. Ensure that your team is reviewing code, but also be aware of the overall tone of the comments. It is hard to be criticized, and perhaps even harder to deliver criticism in a manner that best serves code owner, and ultimately the team. If you begin to see excuses like the ones I’ve mentioned, make sure to have these conversations with your team. Spread the idea of team ownership of the code, mutual respect, and a goal to grow your team’s skills by finding agreed upon best practices.