33

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.

  • 1
    Comments are not for extended discussion; this conversation has been moved to chat. – DarkCygnus Apr 30 at 0:23
  • 1
    Please elaborate. That's all you have to say. – dan-klasson Apr 30 at 23:57
  • Don't ask for elaboration in comments. Call them. It will save you both time and emotions. – Sulthan May 2 at 6:55
  • I'm wondering why there wasn't an earlier design review at the time that the current method was chosen, before reaching the point of being implemented in code and now being reviewed. – rcgldr May 2 at 10:23
  • Kill him with kindness. Answer: "Thanks for your input. Can you elaborate a little more on that please?" – Cris May 5 at 9:16

10 Answers 10

71

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.

| improve this answer | |
  • 67
    I think the OP should also explain why they thought the other method was better, since they also just asked for a different approach, without saying why. – thursdaysgeek Apr 29 at 14:57
36

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

  1. I can't be bothered.
  2. 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.

| improve this answer | |
  • 12
    +1 for simply adding a second comment and stating that the OPs ego is not a valid reason to delay accepting the PR – Old Nick Apr 29 at 9:52
  • 28
    While I do agree that local culture changes the acceptability of certain phrases, people also need to consider how they communicate with people from other cultures. "No we can't" is unhelpfully terse. If there's a valid reason, provide it, or at least summarize it ("No, we can't because of [X]"). But the absence of any meaningful feedback immediately gives off the vibe of this being a rejection without a good basis. It comes across as someone who doesn't want their code to be challenged, rather than someone who knows that a given suggestion doesn't work. – Flater Apr 29 at 10:10
  • 16
    "It would make the code worse for obvious reasons", the fact that someone has asked about it in PR surely means it's at least not obvious to everyone and there is an opportunity to provide some clarity – tddmonkey Apr 29 at 10:18
  • 16
    @Flater: If the developer was Finnish, then he was unusually talkative. The usual answer would have been "No". – gnasher729 Apr 29 at 13:45
  • 13
    I assume the first paragraph was meant to emphasise that different cultures communicate differently. That is true, but the statement comes across as stereotyping (both cultures) or using culture as an excuse (not that the review comment is particularly problematic, when considered in isolation). It could also apply to different personality types to the same degree. The difference between "Can we" and "We should" seems like nitpicking (although nitpicking can happen). You should ask for clarification even if you suspect they might think something is obvious or stupid. How else will you learn? – Bernhard Barker Apr 29 at 14:43
17

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?

| improve this answer | |
  • 12
    This. If the OP had posted their comment on Stack Overflow, it would have been downvoted into oblivion, closed, deleted, and nuked from orbit in the blink of an eye for being unclear, too broad, and opinion-based. If the OP believes they have an improvement, they should explain why they believe it is an improvement and objective reasons for it. – Jörg W Mittag Apr 29 at 21:58
6

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.

| improve this answer | |
3

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.

| improve this answer | |
  • Maybe I'm wrong, but I read PR as peer review. I've worked for two companies that both use code review and peer review interchangeably. – Ben Apr 29 at 14:01
  • @ben: the OP says “Approving that pull request (PR)”. Peer Review does equal Code Review, but that is not what this is about. – jmoreno Apr 29 at 16:52
  • What makes you think OP wasn't suggesting a change? – tbrookside Apr 29 at 20:11
  • @jmoreno, When I made that comment, the question did not include "pull request". It was edited later by someone who is not OP. My statement stands unless disputed by OP. – Ben Apr 30 at 12:50
  • @Ben: also in a comment by the OP in the comments, since moved to a chat room. – jmoreno Apr 30 at 17:55
2

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.

| improve this answer | |
  • Agreed that ego should never stop one from approving the PR. In fact, i have approved it. thanks for your valuable time. – boredbear153 Apr 29 at 10:29
  • Unfortunately none of those are good enough reasons to approve code if you do not understand why the author made the decisions that they did. What if the function you're suggesting is better tested and the other function is janky one-off that slipped into the code base? As a manager, I do not want to see the team to cut corners under the pressure to deliver. At the end of the day perhaps the deadline could be renegotiated. – jcmack Apr 29 at 22:49
  • @jcmack depends on the company culture and priorities, but fair point. Will add that caveat. – Matthew Gaiser Apr 30 at 0:26
0

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.

| improve this answer | |
0

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.

| improve this answer | |
-1

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.

| improve this answer | |
  • "Please expound" conveys your superiority just fine... – Alexei Levenkov Apr 29 at 19:31
  • I like to use "could you elaborate?" – Bwmat Apr 29 at 19:34
  • @Bwmat That's another great way to put it. :-) – kmort Apr 29 at 19:44
  • @AlexeiLevenkov That's an interesting perspective. I have never thought of it that way. I guess it really depends upon your pre-existing relationships with your coworkers. In any case, phrase it how you like, but ask for more information. :-) – kmort Apr 29 at 19:46
-2

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.
| improve this answer | |

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .