4 ms·

Here are some solutions I've seen in various teams: For one-off reviews, set the expectation: The developer is asking for feedback, and not permission. He does

Here are some solutions I've seen in various teams: For one-off reviews, set the expectation: The developer is asking for feedback, and not permission. He doesn't need to justify to the reviewer why he didn't incorporate all the changes. Don't give people pointless authority over others. This will solve most of the problems in the submission. For more structured/important reviews, a third party moderator decides on disputed code changes. If you can't have this, then insist on > 1 reviewers and state you'll only make changes if the reviewers have consensus. This often solves style issues. Changes I would like to see, but have not seen: Reviewers should not ask questions - they should state concerns. So no "Why didn't you do this via X?". Instead, say "I think doing it via X instead of Y is better because ... ". And definitely banned: "Why did you solve it this way?"

eyelidlessness 2y ago
Asking questions is often the best part of review IME, at least when the overall review culture is healthy and productive. If nothing else, it’s often a good opportunity to add clarifying comments where something isn’t obvious after it was written. And authors receptive to good review, again IME, often agree that added clarity will help those cases. “Why did you solve it this way?” is also an excellent question in that context. Not for the big “overall approach”, where the common refrain is that design should come before implementation. But sometimes it helps surface smaller areas of complexity that only become obvious in the trenches, and it can be incredibly helpful to socialize those once discovered. (Also often prompting addition of clarifying comments.)
- BeetleB 2y ago
  Asking questions is great if you're genuinely confused or need clarification. But if you have a concern, express it with the question so there is context. > “Why did you solve it this way?” "Because it solved the problem." Why are you asking me this question if you have no concerns? If you have some, why aren't you sharing them with me? If you're not going to, then I will give the minimally correct answer to your question. Conversations are a 2-way street. If you want me to spend a lot of time explaining it, then reciprocate! Anyone can ask open ended questions. It takes a lot more work to answer them than to ask. Make it worth my time to answer them! Forcing reviewers to put an effort in the discussions also solves a lot of problems in this submission. People aren't going to waste a lot of their own time playing these kinds of games. Make the barrier high enough to prevent 90% of bad actors from acting.
  - eyelidlessness 2y ago
    This all seems very situational to me, starting with level of trust/team cohesion/etc. Like, if I’m on a team that’s functioning poorly together, I almost certainly agree with you without any further nuance. If I’m on a team that’s functioning well, however—taking the example of “why did you solve it this way?”—I might not always know why I’m being asked, but merely being asked is often good enough for me to come up with some hypotheses. If it’s not, there’s a good chance it’s a learning opportunity: maybe something is missing from my toolbelt, or perhaps some unintended consequence escaped my notice. Being prompted to think about those possibilities without bias may help me learn (or reinforce what I’ll learn) by putting me in a position to retrace my steps and look for other gaps in my understanding. I say all this also acknowledging that I tend to put a lot more effort into review, and communication in a review context, than many of my peers have throughout my career. And sometimes my tendency to over communicate has had the opposite effect from what I intended. In any case, I am definitely more inclined toward your position in context of other communication challenges!
    - BeetleB 2y ago
      > I might not always know why I’m being asked, but merely being asked is often good enough for me to come up with some hypotheses. If it’s not, there’s a good chance it’s a learning opportunity: maybe something is missing from my toolbelt, or perhaps some unintended consequence escaped my notice. Being prompted to think about those possibilities without bias may help me learn (or reinforce what I’ll learn) by putting me in a position to retrace my steps and look for other gaps in my understanding. I'm not at all disagreeing that these things may happen, with emphasis on may. It can all be achieved by the code reviewer saying "Interesting approach. You could have solved it via X instead, which has these extra benefits. Was there a reason you picked Y over X?" This is a 2-way conversation. In your paragraph above, you're not giving serious enough consideration that it may really be the reviewer who needs to learn something new, and therefore the conversation should reflect that symmetry. I've often seen (yes, in low functioning teams), the following pattern: Reviewer: Why did you solve the problem this way (X)? Me: Because of (outline a simple reason). Reviewer: But why not Y? Me: Why Y? Reviewer: (50% of the time does not give a good answer and merely insists Y is better because of some random article on the Internet). Note that: 1. The reviewer can't articulate why his method is superior. 2. The developer had to put in a lot more effort in this exchange than the reviewer. In a better functioning team, it goes like: Reviewer: Why did you solve the problem this way (X)? Me: It worked. Why do you ask? Reviewer: Oh, I thought X would be better. (If I'm lucky he'll explain why). Me: OK. Moving on...? Reviewer: (Moves on or sticks to the topic but gives more context). Both of these are "poor" interactions, compared to the reviewer simply stating up front what his concerns are.
shepherdjerred 2y ago
> He doesn't need to justify to the reviewer why he didn't incorporate all the changes. Don't give people pointless authority over others. This will solve most of the problems in the submission. IMO this is a sign of an unhealthy/mistrusting team. Teams often require code to be reviewed, so, yes, you _are_ asking for permission. If a reviewer offers feedback that you feel is unimportant, you can always ask someone else for approval. Going behind someone's back like that is obviously unhealthy, so you should instead have a discussion with the reviewer about why something is/isn't important. If you feel like you can't have these conversations or the reviewer isn't taking your thoughts into account, then that's a sign that you aren't aligned.
- BeetleB 2y ago
  > Teams often require code to be reviewed, so, yes, you _are_ asking for permission. You conveniently left out: > For one-off reviews, set the expectation: The developer is asking for feedback, and not permission. I'm not speaking about what code reviews are, but what they should be. I've personally seen this policy work. It's not appropriate for all kinds of projects, though. Furthermore, a nitpick: Requiring a code review does not equate to permission. It just means people are required to solicit feedback. > If a reviewer offers feedback that you feel is unimportant, you can always ask someone else for approval. If the team allows for this, then I find it to be totally acceptable. To me, it's the same as: "a third party moderator decides on disputed code changes. If you can't have this, then insist on > 1 reviewers and state you'll only make changes if the reviewers have consensus." > Going behind someone's back like that is obviously unhealthy, so you should instead have a discussion with the reviewer about why something is/isn't important. Agreed, and a solution is: 1. Make code review discussions transparent - at least to management (e.g. visible via a code review tool). This limits the potential for abusive teammates. 2. Don't make them go behind the reviewer's back. Simply invite additional reviewers (or escalate to the moderator). Do it openly so the reviewer is aware. I never said "Don't have conversations". Conversations are necessary. I'm saying "Don't give the reviewer undue influence without proper checks and balances". If he is engaging in the tactics in the submission, ensure your team culture is such that this is open and can be handled. You can see in the other comments how common these problems are. Expecting most teams to be high trust and well functioning is unrealistic. Simple rules, however, can shift the team closer to that goal. > If you feel like you can't have these conversations or the reviewer isn't taking your thoughts into account, then that's a sign that you aren't aligned. Or (and more common), it's a sign that the reviewer is a poor reviewer and/or abusing his power.

eyelidlessness 2y ago

Asking questions is often the best part of review IME, at least when the overall review culture is healthy and productive. If nothing else, it’s often a good opportunity to add clarifying comments where something isn’t obvious after it was written. And authors receptive to good review, again IME, often agree that added clarity will help those cases. “Why did you solve it this way?” is also an excellent question in that context. Not for the big “overall approach”, where the common refrain is that design should come before implementation. But sometimes it helps surface smaller areas of complexity that only become obvious in the trenches, and it can be incredibly helpful to socialize those once discovered. (Also often prompting addition of clarifying comments.)

BeetleB 2y ago
Asking questions is great if you're genuinely confused or need clarification. But if you have a concern, express it with the question so there is context. > “Why did you solve it this way?” "Because it solved the problem." Why are you asking me this question if you have no concerns? If you have some, why aren't you sharing them with me? If you're not going to, then I will give the minimally correct answer to your question. Conversations are a 2-way street. If you want me to spend a lot of time explaining it, then reciprocate! Anyone can ask open ended questions. It takes a lot more work to answer them than to ask. Make it worth my time to answer them! Forcing reviewers to put an effort in the discussions also solves a lot of problems in this submission. People aren't going to waste a lot of their own time playing these kinds of games. Make the barrier high enough to prevent 90% of bad actors from acting.
- eyelidlessness 2y ago
  This all seems very situational to me, starting with level of trust/team cohesion/etc. Like, if I’m on a team that’s functioning poorly together, I almost certainly agree with you without any further nuance. If I’m on a team that’s functioning well, however—taking the example of “why did you solve it this way?”—I might not always know why I’m being asked, but merely being asked is often good enough for me to come up with some hypotheses. If it’s not, there’s a good chance it’s a learning opportunity: maybe something is missing from my toolbelt, or perhaps some unintended consequence escaped my notice. Being prompted to think about those possibilities without bias may help me learn (or reinforce what I’ll learn) by putting me in a position to retrace my steps and look for other gaps in my understanding. I say all this also acknowledging that I tend to put a lot more effort into review, and communication in a review context, than many of my peers have throughout my career. And sometimes my tendency to over communicate has had the opposite effect from what I intended. In any case, I am definitely more inclined toward your position in context of other communication challenges!
  - BeetleB 2y ago
    > I might not always know why I’m being asked, but merely being asked is often good enough for me to come up with some hypotheses. If it’s not, there’s a good chance it’s a learning opportunity: maybe something is missing from my toolbelt, or perhaps some unintended consequence escaped my notice. Being prompted to think about those possibilities without bias may help me learn (or reinforce what I’ll learn) by putting me in a position to retrace my steps and look for other gaps in my understanding. I'm not at all disagreeing that these things may happen, with emphasis on may. It can all be achieved by the code reviewer saying "Interesting approach. You could have solved it via X instead, which has these extra benefits. Was there a reason you picked Y over X?" This is a 2-way conversation. In your paragraph above, you're not giving serious enough consideration that it may really be the reviewer who needs to learn something new, and therefore the conversation should reflect that symmetry. I've often seen (yes, in low functioning teams), the following pattern: Reviewer: Why did you solve the problem this way (X)? Me: Because of (outline a simple reason). Reviewer: But why not Y? Me: Why Y? Reviewer: (50% of the time does not give a good answer and merely insists Y is better because of some random article on the Internet). Note that: 1. The reviewer can't articulate why his method is superior. 2. The developer had to put in a lot more effort in this exchange than the reviewer. In a better functioning team, it goes like: Reviewer: Why did you solve the problem this way (X)? Me: It worked. Why do you ask? Reviewer: Oh, I thought X would be better. (If I'm lucky he'll explain why). Me: OK. Moving on...? Reviewer: (Moves on or sticks to the topic but gives more context). Both of these are "poor" interactions, compared to the reviewer simply stating up front what his concerns are.

shepherdjerred 2y ago

> He doesn't need to justify to the reviewer why he didn't incorporate all the changes. Don't give people pointless authority over others. This will solve most of the problems in the submission. IMO this is a sign of an unhealthy/mistrusting team. Teams often require code to be reviewed, so, yes, you _are_ asking for permission. If a reviewer offers feedback that you feel is unimportant, you can always ask someone else for approval. Going behind someone's back like that is obviously unhealthy, so you should instead have a discussion with the reviewer about why something is/isn't important. If you feel like you can't have these conversations or the reviewer isn't taking your thoughts into account, then that's a sign that you aren't aligned.

BeetleB 2y ago
> Teams often require code to be reviewed, so, yes, you _are_ asking for permission. You conveniently left out: > For one-off reviews, set the expectation: The developer is asking for feedback, and not permission. I'm not speaking about what code reviews are, but what they should be. I've personally seen this policy work. It's not appropriate for all kinds of projects, though. Furthermore, a nitpick: Requiring a code review does not equate to permission. It just means people are required to solicit feedback. > If a reviewer offers feedback that you feel is unimportant, you can always ask someone else for approval. If the team allows for this, then I find it to be totally acceptable. To me, it's the same as: "a third party moderator decides on disputed code changes. If you can't have this, then insist on > 1 reviewers and state you'll only make changes if the reviewers have consensus." > Going behind someone's back like that is obviously unhealthy, so you should instead have a discussion with the reviewer about why something is/isn't important. Agreed, and a solution is: 1. Make code review discussions transparent - at least to management (e.g. visible via a code review tool). This limits the potential for abusive teammates. 2. Don't make them go behind the reviewer's back. Simply invite additional reviewers (or escalate to the moderator). Do it openly so the reviewer is aware. I never said "Don't have conversations". Conversations are necessary. I'm saying "Don't give the reviewer undue influence without proper checks and balances". If he is engaging in the tactics in the submission, ensure your team culture is such that this is open and can be handled. You can see in the other comments how common these problems are. Expecting most teams to be high trust and well functioning is unrealistic. Simple rules, however, can shift the team closer to that goal. > If you feel like you can't have these conversations or the reviewer isn't taking your thoughts into account, then that's a sign that you aren't aligned. Or (and more common), it's a sign that the reviewer is a poor reviewer and/or abusing his power.