How should I handle a blunt response to a code review comment?

Question

My team member and I work in different time zones. I happened to review a particular piece of code and posted a comment asking if we can use a different method instead of the one that was used. The reply was simply

"No, we can't"

I may have not understood the code written while reviewing and even that code could have been a dead simple one. But for some reasons, I am not able to accept this blunt reply. It's not about the seniority, it's about basics, I feel the attitude of the developer in this case is not correct. How can we handle such a situation? Approving that pull request (PR) will hurt my ego but not approving it will delay the ticket completion.

Also, I may appear dumb for seeking an explanation for the above comment. This is a very simple thing, but for some reason, I'm not able to brush this one off. Also, in an earlier interaction. when I asked the same developer for a few clarifications all I got as the response was that I should express my doubts in the PR only.

Answer 1

Just responding to a comment with "No" is generally unhelpful to both the reader and the writer, so I can imagine getting a little annoyed by it. There's a few things I've done in the past to get more information out of people (although I've rarely worked with people who are actively unhelpful)

Remember that the goal of a PR isn't to show off how good you are (on either side), but to make sure the code is solid, maintainable and as good as it's going to be. So respond from that feeling with requests for more information.

My first response question in this case would probably be "Can you explain why it can't be used?" to understand their reasoning. This is important, because if I don't get it, and I need to write a similar feature, I might decide to use that method and then waste time (or worse, create bugs) learning about why I shouldn't have. (You can add however much of that additional explanation as you need).

In a case where I figure it out for myself between the initial comment and the follow-up question, I would just add a comment saying "I think I see why I shouldn't use it; is [this] the reason?" and see if I got it right. That way, the other side doesn't have to fully explain it. Some people really don't like to do that (although it ought to be part of your expected workload in a senior position).

Additionally, if the reason for not using a function is technical and obscure, but the function seems like a logical better fit for the situation, my next follow-up would be "Can we make it more clear from the code why we don't use the other function?".

Since we're trying to make the code as good as it can be, warning any future developers about this obvious fix that won't make things better seems important. The next developer to look at this code won't see the PR and the discussion, and might decide to refactor things to make them better.

If the reason for not using the function should be obvious to people using it and I just didn't know about that feature before, that means I learned something new. So I'd follow it up with a simple "Thanks, I didn't know about this yet.". Yes, even if I figured it out myself, since the other developer was still the reason I looked into it.

You'd be surprised how often people will become less crass and will take time to explain things to you, if you thank them when they teach you new things. And even if they don't, you'll feel better for still being a friendly colleague to them.

Answer 2

Let me guess: You are Indian, and he is Finnish. If he's Finnish, that is a perfectly fine answer :-)

Let's say you made this comment on my code. I would read it, and my thoughts would be one of the following:

1. Oh my god why didn't I think of this myself.
2. It would have been better, but not worth the effort now.
3. It wouldn't be an improvement.
4. It would make the code worse for obvious reasons.
5. It would make the code worse for subtle reasons.
6. The idea is so stupid, I wonder what's going on in your brain.

In cases (4) and (6) I might give that reply. Some people would think

7. I can't be bothered.
8. I'm running out of time for the sprint

and might give you the same reply.

What you do, is add a second comment "Why can't this be done" unless you think it's a matter of case (4) or (6).

Now you say "Approving that PR will hurt my ego and if i do not, it will delay the ticket completion. ". The first is not a valid reason to delay it, and the second is not a valid reason to accept it.

But looking at your post, where you asked "can't we do X instead". Why would we do X instead? Because you prefer it? That's not a good reason. Because we can? That's not a good reason to change the code. Because it is better? Because X is more reliable, better documented, faster, cheaper whatever? In that case your comment should have been "We should change this to do X because ... " followed by a good reason. If we ask "Can we change this? " then "No" is not an unreasonable answer.

Answer 3

There are loads of good answers about what to do in the current situation. I want to focus on how to avoid getting into the situation at all.

posted a comment asking if we can use a different method instead of the one that was used

This is a yes-or-no question/comment. As such the answer you got is perfectly fine. It would even be perfectly fine to answer it as "yes", but without doing any change. After all the comment asks if it's possible, it's not recommending the change. Now, all of this is based on the actual wording, and the intention of your comment clearly isn't to get a yes or no answer.

There can be several reasons the developer didn't explain the answer, I won't go into possible reasons. We just need to understand this can happen.

How can you make sure to get explanations to questions? It's actually quite easy, don't ask yes-or-no questions. It takes some thought to write comments in a good way, but it's a good skill to have.

Bad comment: Can we use method ABC instead of current method?

Better comment: I think this could be improved by using method ABC. Why are we using the current method?

That comment is better, since it posts an open question. But there are still a few problems with it. We don't give context on why we think ABC is better than the current method. And we are putting the blame on the developer, making her defensive. We should instead put it in a way where we assume there is a good reason, and we just don't understand it. It makes it easy for the developer to either explain or to admit they did a mistake.

Good comment: I think this could be improved by using method ABC because DEF. We are already using it in place GHI. I don't understand the reason for the current choice, can you please explain?

Answer 4

They may just be answering in a passive-aggressive way to a passive-aggressive question.

Busy developer gets code comment. “Could” we use a different method? Well yeah, no crap, I bet we could, unless what you’re suggesting is just plain wrong. So do I need to spend my valuable time on a long defense of the method I used? No, I will spend as much effort as the questioner clearly took. I’ll say “No.” Or “yes” but not make any change.

If you want a more constructive response make a more constructive comment. “I think method X would work better here for these reasons, explanation follows. Change if you want|this should be changed because yours is provably wrong|other call to action.”

Busy developers get real sick of minimal code review prompts like “could you have done something else” or “is this the best it could be?” Make concrete suggestions with reasons.

Answer 5

You use code review and PR (Pull Request) as if they are interchangeable, they aren’t. A PR can include a code review but is fundamentally something different.

A code review is a critique and review of the code, you can do this for old code or new code, code that is intended to be used or code that is just developed to understand the process and will be thrown away. It is used to give feedback about the code or to review and learn about the code. It can be used as part of a gate keeping process, but is not itself about keeping code from moving from one place to another.

A PR is all about moving code from one place to another. It can require more than one person to approve, and that person may or may not be the person that wrote the code, but it is basically the question “can this code go there”.

When dealing with a PR your duty is to make that decision (even if policy requires a dozen other people to agree before it happens). You ask questions and make suggestions in order to make that decision. If an answer allows you to make the decision then great, if not ask another question until you can make the decision.

You should respond to your co-worker making it clear that you aren’t suggesting a change, but are instead seeking understanding. And that you aren’t going to approve the request until you have that understanding. The co-worker doesn’t have to be the one that helps you gain that understanding.

Answer 6

The environment matters a lot here

Code review is one of those things dramatically shaped by other factors.

• Is the sprint ending?
• Is it a bug that someone in management wants fixed urgently?
• Are there actually standards or is code review just a wild west of opinions?
• Are there code production quotas?
• Is someone in QA asking for things to test?

All of these things tend to make developers more hostile to code review. Are any of these present? That may be why the dev did not indulge you.

If you don't have any concerns about the code and your question is one mostly of curiosity, I would be inclined to approve it, but whether these reasons are acceptable or not will depend on the company culture and what they prioritize. Your ego is not a good reason to hold up the ticket.

Answer 7

If you have a suggestion, and your suggestion waved away bluntly, there can be a few reasons. But there is a reason that I think is overlooked.

It may be possible that the reason you suggestion doesn't work is rather obvious. The team member may be afraid of patronizing you in a place where others can see. They are giving you a chance of your own accord to realise the reason, which allows you to save face.

It would take an understanding of the mentality of the team member to know if this is the underlying reason.

The very first thing you should do is reread the review carefully and try to see if there is anything obvious you missed.

If you still can't figure it out, you can ask a follow up question in the review. Even if you are wrong, that may plant a seed of doubt in the team members mind. They may realise that what is in their head is not so obvious, which may lead them to inserting a comment in the code to explain why the "obvious" solution doesn't work, to avoid someone down the track from "improving" the code.

It always helps to leave the ego behind, and also, as much as you can, assume a bit of good faith with your coworkers. I work in the software development field, and I can safely say most people I've worked with have absolutely no malice in their actions. I hope your experience is the same.

Answer 8

The original question posed indicates that the code "smells", at least to the nose of the person asking the question.

If the logic has to be implemented in that particular way, then a code comment explaining this will help future developers understand why.

It may be that in the future, a developer who is troubleshooting an issue will come across this smelly code and decide that somehow it is the cause of the bug, and attempt to refactor it as the fix. They will do this even if the connection between the smelly code and the observed problem is not clear; refactoring smelly code is low-hanging fruit when correcting a bug, and sometimes it works.

Also, if the question was posed once, it will be posed again by someone who for some reason is not aware that it has already been posed.

So to save time in the future, invest some time now and thoroughly comment the code.

Answer 9

Great question. Technically, they answered your question, but you want more information, so just ask for it.

I have found a simple similarly-blunt reply of "Please expound" to be very helpful in such situations.

Often, these seemingly-blunt "No, we can't" responses are when the developer is busy or is thinking about something else. You just want to bring their attention back to the issue you have. Your simple response of "Please expound" conveys no judgement, is very brief and asks for more information, putting the ball back in the developers court. It conveys to them this question requires more than a three-word answer.

In my experience, you'll get a much better answer this next time around. Good luck.

Answer 10

I don't like comments in code, for various reasons. So I have developed a set of rules for when a comment is permitted. Generally, documentation comments are fine, but "line" comments are bad.

One exception to this generalization is to provide "negative information."

That is, comments that provide "positive" information about what you are doing generally should not exist, since the code should be clear enough. (When the code is unclear, and has to stay unclear, you may comment.)

But when there is "negative" information, that is, information about things you are deliberately not doing, or things you tried but couldn't use, etc., that justifies a comment since there's no way to put that into the source code.

If your question is literally true -- if you asked about using an alternate method, and it's not permissible or not possible to use that method for some concrete reason -- and furthermore if you, an experienced developer, were initially in favor of changing the code to use that method, then that justifies a comment under the negative information exception:

// NB: sortUsingDefaultKey() won't work here because
// this function has to be re-entrant.