7 ms·

There's basically two categories here: - you have an opinion on how things ought to be done and want a dialog - you see some code that's wrong or violates an

There's basically two categories here: - you have an opinion on how things ought to be done and want a dialog - you see some code that's wrong or violates an agreed-upon rule and so it should be changed with no discussion I'd switch the tone based on what you're addressing. Giving a rationale when you share an opinion or point out a mistake also softens the tone (for the better IMO). >* Should we extract this to a separate function? >* Could you extract this to a separate function? These are essentially the same: opening a dialog over an opinion. I'd suggest this when there's not an obvious flaw or rule violation or you have a gut-feeling about how code should be and want the author's input. >* I would extract this to a separate function. This is almost a command but isn't clearly one. It should be followed by some rationale, at least. >* This could be extracted to a separate function. This one's the least useful. You could do a lot of things with code. It doesn't resemble the command or inviting tones of the other examples here. >* This should be extracted to a separate function. >* Extract this to a separate function. These are commands and are practically the same tone and best when catching mistakes. Unless obvious, a rationale should be given like a demonstrable flaw in logic or inconsistent abstraction, etc. There should be a few sentences explaining this. If there isn't a clear violation, I'd prefer the dialog invitation tones.

edmcnulty101 4y ago
This is really useful. Thanks.
jvanderbot 4y ago
Another thing that people don't often consider is that while some really appreciate being given a pointer to the rule as a justification (these are in a wiki / code style doc usually), many people will chafe at the "pedantry". In general, those second group need a bit thicker skin, but it is something to be considered.
m3Lith 4y ago
What about clarifications? In a sense when you're not sure if this is good code, and would need to get more details. I sometimes just ask directly ("Why is this there?"/"What does this do?"), but other times I just don't bother and let others review.
matsemann 4y ago
I also think it depends a bit on the other person and the rapport one has with them. At least I try to remember back to when I was a junior, and how "personal" the feedback felt when I was just starting. So when reviewing for others in the same situation I make extra sure it doesn't come off the wrong way by adjusting my tone. But for someone I've worked with for years we're often a bit shorter and to the point, and everyone's happy.
- ginja 4y ago
  Yeah I remember being a junior and feeling like blunt comments were kind of rude. So I make an effort to be nicer to new developers - sometimes simply adding “what do you think?”/“do you agree?” is enough to make those command style sentences sound much less blunt, and also encourages them to ask questions instead of blindly following my suggestion if they don’t fully understand it.
  - graftak 4y ago
    To me those questions could come off as condescending. I say could because I wouldn’t think that’s your intent, but the tone is there.
    - kqr 4y ago
      That depends entirely on whether you add the question as a formality or if you are genuinely interested in why the other person might disagree.
      - graftak 4y ago
        To me ’…don’t you agree?’ is the sort of rhetoric that parents use to make their kids internalise their beliefs. If someone disagrees I expect them to speak up, if they’re not so inclined I’m interested to know why, so we can address/resolve that and move on.
        Kaze404 4y ago
jorgeleo 4y ago
<sarcasm> I like your suggestions... I would add that they are more effective if you keep a background noise with whips cracking sounds and random screams </sarcasm>
ajross 4y ago
> I'd switch the tone based on [whether you want a discussion or are insisting on a change] I think that's a great example of where "tone" is absolutely not sufficient, especially in the modern world where many readers aren't going to share your first language and won't absorb the nuance. If there's a situation where your disagreement is absolute, you need to say that factually, ideally with a recipe for how to resolve that included: "I can't approve this scheme, please do X instead." "This API isn't OK to use here, you have to..." "Under no circumstances will I ever approve of a feature that does this." You can phrase those as nicely as you want, but it's imperative that your disagreement be spelled out explicitly.
- colonwqbang 4y ago
  It's sometimes refreshing to have people just write what they want me to do, clearly and without embellishment. Even if it's sometimes nitpicking, I know that if I just fix it I can then merge my patch. I don't have to wait for a second look from them because their comments were so direct "write this instead".
kevinpet 4y ago
There are different categories of things you might see: 1. This is wrong. I can tell from reading your code that it doesn't do what the description of this PR or the name of a test or some other indicator shows its supposed to do. This should be communicated in a tone of "you must not merge this". 2. This violates our agreed-upon style or best practices. The strictness of enforcement is part of that agreement and company culture. At my current company, this would be communicated along the lines of "this is not how we prefer to do this, so unless you have a good reason why our standard method is wrong, change it" 3. This is confusing to me, or I have a suggestion for improvement. This should be communicated as a suggestion or general feedback. If you're getting miffed about people not taking the suggestions, maybe it's actually in a category above, or maybe you need to adjust your own assumptions about how much stylistic consistency to insist on, since it sounds like you don't have a consensus. 4. You are new, either to software engineering or at least to this company, and your style is inconsistent with how things are done in this code. This is the same as #3 but should be communicated more strongly and depending on your company normals may be effectively a requirement.
HWR_14 4y ago
>>* This could be extracted to a separate function. > This one's the least useful. You could do a lot of things with code. It doesn't resemble the command or inviting tones of the other examples here. It's clearly a politely phrased command, and to think otherwise is intentionally missing the point.

edmcnulty101 4y ago

This is really useful. Thanks.

jvanderbot 4y ago

Another thing that people don't often consider is that while some really appreciate being given a pointer to the rule as a justification (these are in a wiki / code style doc usually), many people will chafe at the "pedantry". In general, those second group need a bit thicker skin, but it is something to be considered.

m3Lith 4y ago

What about clarifications? In a sense when you're not sure if this is good code, and would need to get more details. I sometimes just ask directly ("Why is this there?"/"What does this do?"), but other times I just don't bother and let others review.

matsemann 4y ago

I also think it depends a bit on the other person and the rapport one has with them. At least I try to remember back to when I was a junior, and how "personal" the feedback felt when I was just starting. So when reviewing for others in the same situation I make extra sure it doesn't come off the wrong way by adjusting my tone. But for someone I've worked with for years we're often a bit shorter and to the point, and everyone's happy.

ginja 4y ago
Yeah I remember being a junior and feeling like blunt comments were kind of rude. So I make an effort to be nicer to new developers - sometimes simply adding “what do you think?”/“do you agree?” is enough to make those command style sentences sound much less blunt, and also encourages them to ask questions instead of blindly following my suggestion if they don’t fully understand it.
- graftak 4y ago
  To me those questions could come off as condescending. I say could because I wouldn’t think that’s your intent, but the tone is there.
  - kqr 4y ago
    That depends entirely on whether you add the question as a formality or if you are genuinely interested in why the other person might disagree.
    - graftak 4y ago
      To me ’…don’t you agree?’ is the sort of rhetoric that parents use to make their kids internalise their beliefs. If someone disagrees I expect them to speak up, if they’re not so inclined I’m interested to know why, so we can address/resolve that and move on.
      - Kaze404 4y ago

jorgeleo 4y ago

<sarcasm> I like your suggestions... I would add that they are more effective if you keep a background noise with whips cracking sounds and random screams </sarcasm>

ajross 4y ago

> I'd switch the tone based on [whether you want a discussion or are insisting on a change] I think that's a great example of where "tone" is absolutely not sufficient, especially in the modern world where many readers aren't going to share your first language and won't absorb the nuance. If there's a situation where your disagreement is absolute, you need to say that factually, ideally with a recipe for how to resolve that included: "I can't approve this scheme, please do X instead." "This API isn't OK to use here, you have to..." "Under no circumstances will I ever approve of a feature that does this." You can phrase those as nicely as you want, but it's imperative that your disagreement be spelled out explicitly.

colonwqbang 4y ago
It's sometimes refreshing to have people just write what they want me to do, clearly and without embellishment. Even if it's sometimes nitpicking, I know that if I just fix it I can then merge my patch. I don't have to wait for a second look from them because their comments were so direct "write this instead".

kevinpet 4y ago

There are different categories of things you might see: 1. This is wrong. I can tell from reading your code that it doesn't do what the description of this PR or the name of a test or some other indicator shows its supposed to do. This should be communicated in a tone of "you must not merge this". 2. This violates our agreed-upon style or best practices. The strictness of enforcement is part of that agreement and company culture. At my current company, this would be communicated along the lines of "this is not how we prefer to do this, so unless you have a good reason why our standard method is wrong, change it" 3. This is confusing to me, or I have a suggestion for improvement. This should be communicated as a suggestion or general feedback. If you're getting miffed about people not taking the suggestions, maybe it's actually in a category above, or maybe you need to adjust your own assumptions about how much stylistic consistency to insist on, since it sounds like you don't have a consensus. 4. You are new, either to software engineering or at least to this company, and your style is inconsistent with how things are done in this code. This is the same as #3 but should be communicated more strongly and depending on your company normals may be effectively a requirement.

HWR_14 4y ago

>>* This could be extracted to a separate function. > This one's the least useful. You could do a lot of things with code. It doesn't resemble the command or inviting tones of the other examples here. It's clearly a politely phrased command, and to think otherwise is intentionally missing the point.