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.
halifaxdev 
If you are hearing these a lot, then they are symptoms of other problems.
Take the example, “Why does this function return null?” and the answer
For “It’s not MY code!” – you can read this as, “the code base is big, I don’t know everything that is in the code base at all, and I certainly don’t understand this part.” and the answer “No idea, not my code!”. You are taking the “not my code” part to heart. maybe you should focus on the “no idea” part.
Sometimes code-bases get big, teams get big. Not everyone knows everything about the code. “Why does this function return null?” – Well maybe there is a really good reason, _but I don’t know it_. It’s a valid response. The ideal is that the team owns the codebase, but take a nod to reality, none of the developers will understand the entire codebase. Accept that, don’t get angry that they don’t know, go ask the person who does.
Or it is a sign that the review process is turning adversarial. Who wants to be roasted for someone else’s bad code?
In which case, make sure the reviewer and devs know what’s up.
As for “That section isn’t finished yet!” – That is a sign your project is being rushed. Don’t roast the dev over it, find out why the process you are using is forcing you to review the code too early. Is the project behind and people are pushing stuff to be pushed in early (a common cause)? Is you code pipeline wrong (it is checked in, it must be reviewed right now!)? Are you reviewing just before release, and the release schedule too tight?
The rest are pretty fair points.
LikeLike
I agree with all your points Blair. For the “Not my code” excuse, I agree with your interpretation, and absolutely it requires follow up. That is the point I was trying to make, where “It’s not my code” may be a fair point, but it becomes a poor excuse when the conversation stops there. Followup needs to occur so the team can find a good solution to the concerns.
I also agree with your assertion that the “That section isn’t finished yet” is potentially a process issue. Either way, it should be addressed. For all these comments, they really only become bad excuses when they are used as a way to end a conversation. I am glad you make that point.
Cheers
LikeLike
When dealing with older code bases, it’s typical for reviewers to find issues “outside the scope” of the review potentially resulting in either a “not my code” or “that section is not under review” excuse. As you said, follow up is key. Instead, consider replying with “I’m not sure why it was implemented like that. Let me investigate and get back to you.” If the change is smallish and easy to understand, just fix it, or open a bug/task to investigate and address later.
LikeLike