9 ms·

Excessive subjective feedback can be soul-crushing.

acscott 3y ago
Agreed; after > 20 years of coding successfully, got hit by a storm of subjective feedback; it totally ruined any joy in development
- TheBlight 3y ago
  It didn't used to be this way. A code review used to mostly function as a quick sanity check. Now it's basically code by committee.
- wubrr 3y ago
  The fun thing to do in these situations is to add yourself as reviewer to all PRs by the person giving such feedback and return the favor. They learn pretty fast.
  - runlevel1 3y ago
    That seems like it risks creating conflict out of what's often just a misunderstanding. Assuming it's a corporate environment (it's fuzzier in the open source bazaar): If it's the first time or I don't really know the reviewer, I ask them to hop on a call to discuss (usually to walk me through) their feedback and I go in with an open mind. That gives me the opportunity to find out if I'm missing some context, can see how reasonable they are, and can get clarification of what they actually care about versus FYIs/suggestions. As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion. If everything is a hard opinion and they don't seem reasonable, I reach out to someone else (ideally a team lead or peer on their team) over a private channel for a 2nd opinion. If they also think it's unimportant stuff, I ask them to add their own comments to the PR. Give it a reasonable amount of time and they'll either have reached a consensus or you can roll the side you agree with. If it's an issue again later and they seem reasonable, respectfully push back. If they seem unreasonable, skip right to DMing their lead for a 2nd opinion. If it keeps being in issue, then some frank conversations need to happen. Something I've noticed about folks who steadfastly focus on minor stylistic nits in CRs is they (1) tend to be cargo culting them without understanding the why behind them and (2) they're usually missing the forest (actual bugs in logic) for the trees. Most people are pretty reasonable when they don't feel like they're under attack, so in my experience it's usually possible to resolve these things without dragging it out. Of course, if you're at a company with a lot of disfunction, well... I can understand why what I've written above won't work.
    - wubrr 3y ago
      If it's the first time - yeah, reach out to the person and talk to them. But if the person is consistently leaving such comments under the guise of mentorship, 'raising the bar', or some other bullshit which boils down to them attempting to demonstrate their own seniority at the expense of other people's time and stress - then showing them how it feels is a great approach. Bringing in other people and managers is not very effective I've found - it takes additional time, other people have their own stuff to focus on, and managers often don't have the technical expertise or confidence to push back against subjective comments which claim to be 'raising the bar' or whatever. It also doesn't look great when you have to bring other people in to help you address PR comments. And, of course, you do this without ostensibly creating any conflict - if they complain simply respond along the lines of 'I totally love the care and attention to detail you bring when reviewing my PRs, I've learned from you and thought it would be appropriate to keep the same high standards and not lower the bar... etc'. > As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion. Nah, if they don't explicitly state that's a soft opinion via 'nit', approving with comment or some other means, they are disrespecting the person who's PR they are reviewing. I shouldn't have to chase them down to see how strong their opinions are. > If it keeps being in issue, then some frank conversations need to happen. Something I've noticed about folks who steadfastly focus on minor stylistic nits in CRs is they (1) tend to be cargo culting them without understanding the why behind them and (2) they're usually missing the forest (actual bugs in logic) for the trees. What do you do if the comments are purely subjective and all backed up by internal/corporate dogmaspeak? 'Raising the bar' ... 'keeping the standards high' ... 'mentoring', etc. , or open ended comments asking to explain how stuff works, and whether 'this approach is the best'? There is no shortage of rhetorical bullshit that can be used to justify subjective PR comments. > Most people are pretty reasonable when they don't feel like they're under attack, so in my experience it's usually possible to resolve these things without dragging it out. The above will generally not work in a company that emphasizes PR comment count as a good metric for promotions/performance, and has a lot of internal rhetorical dogma. You WILL get people who leave these types of comments because they view it as a way of promoting their career, these people often cannot be reasoned with logically because they aren't actually all that smart, and they view any pushback against their comments as an attack against them. Other people's feedback against the bullshit comments definitely help, but can look bad if you keep reaching out to other people to help address PR comments - I made sure to go through other people's PRs, on my own initiative, and refute bullshit comments when I realized how some people were behaving. And yeah it totally depends on the company/team.
- cutemonster 3y ago
  Sounds as if that happened suddenly ("got hit"), what changed (in the workplace) that made such bad things possible? (After not having happened for 20 years) How long did it take until you felt better again? If I can ask
MarkLowenstein 3y ago
Anything subjective that doesn't fix an identifiable execution problem must be explicitly labeled as a suggestion. You don't get first choice over the code because you are asked to be the reviewer. You are there to (1) catch mistakes and (2) teach the other coder if they appear to not know something useful that you do know. If you have a preference about a simple stylistic matter that is not covered by your style guidelines, either put it in the guidelines, or hold your tongue.
- zeroCalories 3y ago
  IMO you should never provide feedback that can be implemented as an automated check. If you don't like deeply nested control flow, then you should catch that with static analysis. If you need code coverage, you should require it for merging. Implement your check and provide a new PR to fix your nitpicks, or shut up. The goal is to put 100% of the focus on correctness.
  - merb 3y ago
    Sadly typos in variable names are not checkable that easy
    - zeroCalories 3y ago
      Doesn't seem hard to me? You could easily run a spell checker on identifiers and comments. It will produce a lot of false-positives, but that can be solved by making the changes optional or using an allow list.
      - bluGill 3y ago
        Then please write one. Note that the important part is a good way to mark all those false positives that is simple and doesn't go to far. Just because I want a bad spelling in one place doesn't mean I want it everywhere. As a result I'm going to predict that your tool either results in too much boilerplate needed to suppress all the false positives, or your tool lets pass a lot of things that shouldn't. But that might be just that I don't have good ideas: if you create a good tool for this I'm willing to be proven wrong.
layer8 3y ago
Unlike excessive objective feedback?
- deleted 3y ago
  [deleted]

Agreed; after > 20 years of coding successfully, got hit by a storm of subjective feedback; it totally ruined any joy in development

TheBlight 3y ago
It didn't used to be this way. A code review used to mostly function as a quick sanity check. Now it's basically code by committee.
wubrr 3y ago
The fun thing to do in these situations is to add yourself as reviewer to all PRs by the person giving such feedback and return the favor. They learn pretty fast.
- runlevel1 3y ago
  That seems like it risks creating conflict out of what's often just a misunderstanding. Assuming it's a corporate environment (it's fuzzier in the open source bazaar): If it's the first time or I don't really know the reviewer, I ask them to hop on a call to discuss (usually to walk me through) their feedback and I go in with an open mind. That gives me the opportunity to find out if I'm missing some context, can see how reasonable they are, and can get clarification of what they actually care about versus FYIs/suggestions. As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion. If everything is a hard opinion and they don't seem reasonable, I reach out to someone else (ideally a team lead or peer on their team) over a private channel for a 2nd opinion. If they also think it's unimportant stuff, I ask them to add their own comments to the PR. Give it a reasonable amount of time and they'll either have reached a consensus or you can roll the side you agree with. If it's an issue again later and they seem reasonable, respectfully push back. If they seem unreasonable, skip right to DMing their lead for a 2nd opinion. If it keeps being in issue, then some frank conversations need to happen. Something I've noticed about folks who steadfastly focus on minor stylistic nits in CRs is they (1) tend to be cargo culting them without understanding the why behind them and (2) they're usually missing the forest (actual bugs in logic) for the trees. Most people are pretty reasonable when they don't feel like they're under attack, so in my experience it's usually possible to resolve these things without dragging it out. Of course, if you're at a company with a lot of disfunction, well... I can understand why what I've written above won't work.
  - wubrr 3y ago
    If it's the first time - yeah, reach out to the person and talk to them. But if the person is consistently leaving such comments under the guise of mentorship, 'raising the bar', or some other bullshit which boils down to them attempting to demonstrate their own seniority at the expense of other people's time and stress - then showing them how it feels is a great approach. Bringing in other people and managers is not very effective I've found - it takes additional time, other people have their own stuff to focus on, and managers often don't have the technical expertise or confidence to push back against subjective comments which claim to be 'raising the bar' or whatever. It also doesn't look great when you have to bring other people in to help you address PR comments. And, of course, you do this without ostensibly creating any conflict - if they complain simply respond along the lines of 'I totally love the care and attention to detail you bring when reviewing my PRs, I've learned from you and thought it would be appropriate to keep the same high standards and not lower the bar... etc'. > As they go, if it isn't clear, I just ask them if something is a soft opinion or hard opinion. Nah, if they don't explicitly state that's a soft opinion via 'nit', approving with comment or some other means, they are disrespecting the person who's PR they are reviewing. I shouldn't have to chase them down to see how strong their opinions are. > If it keeps being in issue, then some frank conversations need to happen. Something I've noticed about folks who steadfastly focus on minor stylistic nits in CRs is they (1) tend to be cargo culting them without understanding the why behind them and (2) they're usually missing the forest (actual bugs in logic) for the trees. What do you do if the comments are purely subjective and all backed up by internal/corporate dogmaspeak? 'Raising the bar' ... 'keeping the standards high' ... 'mentoring', etc. , or open ended comments asking to explain how stuff works, and whether 'this approach is the best'? There is no shortage of rhetorical bullshit that can be used to justify subjective PR comments. > Most people are pretty reasonable when they don't feel like they're under attack, so in my experience it's usually possible to resolve these things without dragging it out. The above will generally not work in a company that emphasizes PR comment count as a good metric for promotions/performance, and has a lot of internal rhetorical dogma. You WILL get people who leave these types of comments because they view it as a way of promoting their career, these people often cannot be reasoned with logically because they aren't actually all that smart, and they view any pushback against their comments as an attack against them. Other people's feedback against the bullshit comments definitely help, but can look bad if you keep reaching out to other people to help address PR comments - I made sure to go through other people's PRs, on my own initiative, and refute bullshit comments when I realized how some people were behaving. And yeah it totally depends on the company/team.
cutemonster 3y ago
Sounds as if that happened suddenly ("got hit"), what changed (in the workplace) that made such bad things possible? (After not having happened for 20 years) How long did it take until you felt better again? If I can ask

MarkLowenstein 3y ago

Anything subjective that doesn't fix an identifiable execution problem must be explicitly labeled as a suggestion. You don't get first choice over the code because you are asked to be the reviewer. You are there to (1) catch mistakes and (2) teach the other coder if they appear to not know something useful that you do know. If you have a preference about a simple stylistic matter that is not covered by your style guidelines, either put it in the guidelines, or hold your tongue.

zeroCalories 3y ago
IMO you should never provide feedback that can be implemented as an automated check. If you don't like deeply nested control flow, then you should catch that with static analysis. If you need code coverage, you should require it for merging. Implement your check and provide a new PR to fix your nitpicks, or shut up. The goal is to put 100% of the focus on correctness.
- merb 3y ago
  Sadly typos in variable names are not checkable that easy
  - zeroCalories 3y ago
    Doesn't seem hard to me? You could easily run a spell checker on identifiers and comments. It will produce a lot of false-positives, but that can be solved by making the changes optional or using an allow list.
    - bluGill 3y ago
      Then please write one. Note that the important part is a good way to mark all those false positives that is simple and doesn't go to far. Just because I want a bad spelling in one place doesn't mean I want it everywhere. As a result I'm going to predict that your tool either results in too much boilerplate needed to suppress all the false positives, or your tool lets pass a lot of things that shouldn't. But that might be just that I don't have good ideas: if you create a good tool for this I'm willing to be proven wrong.

layer8 3y ago

Unlike excessive objective feedback?

deleted 3y ago
[deleted]