4 ms·

Eh, this is so contextual though. In the context of a code review, as in the article: no, please point out an issue if you see it, don't hold back. But raise it

Eh, this is so contextual though. In the context of a code review, as in the article: no, please point out an issue if you see it, don't hold back. But raise it once, be willing to let it go, and respect that your colleagues don't have to act on your advice. The golden rule will get you far.

pimlottc 5y ago
Even with a code review, though, you might want to wait until you’ve digested the rest of the review instead of firing off a comment. Maybe it makes sense in context. Or maybe there are bigger fish to fry and it’s not worth quibbling over small things.
- nomel 5y ago
  > Or maybe there are bigger fish to fry and it’s not worth quibbling over small things. I've taken this approach with one of our engineers, and now we have a huge pile of "small things" that has created some pretty serious technical debt. My perception is that they don't try to understand what I'm pointing out, and come up with rational for how they have it. I think I have a lot to learn.
  - jka 5y ago
    I'm still learning too, but I think that something I'm trying to develop is a sense for the longer-term costs and implications of apparent defects. - Is this a component that every engineer is going to interact with and that will stay with the company for decades? - Can this technical decision be reversed easily? Either in the sense of an individual commit revert, and/or in terms of a series of code changes? - Is this change likely to remain isolated to this part of the codebase, or will the pattern infect or be copied to other locations widely? It's super challenging -- and also intellectually rewarding when you can master it and gather team consensus and alignment.
    - hinkley 5y ago
      > Can this technical decision be reversed easily? One of the most exhausting failure modes that I've had to deal with is deciding not to decide. Some people will build elaborate Rube Goldberg machines so that they "can change their minds later" but in fact what they've done is decided that choices are now everybody else's problems. There's a whole hell of a lot of rewards to having the maturity to be able to say "I was wrong" and move on. Writing code in a way that you could rework it to use a different library to accomplish the same thing is making a reversible decision. Creating a rules engine to let a config file make the choices and leaving a cartesian product of states (75% of which are invariably illegal, which you are just supposed to know) is definitely, definitely not. Just pick something, man.
  - jrockway 5y ago
    I definitely have a list of things I've let go just to be nice that have caused production outages, consistent poor performance, time-consuming technical debt, etc. It's great to have a good relationship with your team, but at the end of the day, the things that you "let go" can become reasons not to buy your product or why new engineers won't be productive on the codebase. It's very easy to be the person that never says "no" to something, but at the same time, it's also easy to be the reviewer that doesn't advance the reviewee's career. A reviewer should ask themselves; "by not commenting on this, am I making the reviewee a better programmer?" Something I've noticed is that acquaintances will be nice to you universally, but actual friends know when to broach subjects that may not be considered "pleasant" or "nice". Don't give your friends "acquaintance-level" code reviews.
  - no_wizard 5y ago
    I have found personally only pair programming with people regularly is the only way you can avoid adding lots of technical debt. Technical debt isn't always avoidable, but I sure know the difference from when teams are heavily invested in pair programming vs not. Good pair programming is hard to ingrain in a culture though.
  - babyshake 5y ago
    It is tough to balance between sweating the details and not bikeshedding (sweating the details at the expense of the big important architectural decisions)
  - hinkley 5y ago
    I think it's a matter of picking a few things you're going to comment on, and any time the person actually stops needing to be corrected on one of them, you add one of the others in. Any good teacher or mentor does this. But it's a balance because you need the student to see that the reason you're complaining about something new is because they have in fact progressed, and we have moved on to the next thing to address (which has probably always been there)
- mwcampbell 5y ago
  > Or maybe there are bigger fish to fry and it’s not worth quibbling over small things. This was my excuse to not be thorough when giving feedback in code review, when I was last in a position to do so, while on the Windows accessibility team at Microsoft. I told myself I was on a mission to make Windows more accessible, and there was no time to delay new features or fixes over inconsequential things like coding style or even code organization, as long as it worked. Edit: At least I didn't use that excuse when responding to coworkers' reviews of my PRs.
- renewiltord 5y ago
  This is a tooling problem. Github reviews vs. Bitbucket reviews will show this. In Github you can write your comments and send them all at once. Until you submit the review, you can just edit your comments and undo them or whatever.
- Lockyy 5y ago
  This is why I am so grateful for GitHub reviews; Being able to group comments to fire off all at once has saved me from this multiple times as I continue reading, realise something, and remove a previous comment. In fact you reminded me of the technique right now to check the responses and make sure nobody else had already said this!
  - spockz 5y ago
    On the other hand, someone coming after you might read the code in the same order and have the same wtf moment. They may even never find that later piece you saw in the review. So at the least it should still be a signal to the Author that it appears strange.
    - funkymike 5y ago
      I also really like this feature of GitHub reviews and when deleting a comment as I continue reading I try to question whether to add a code comment so the next person doesn't get tripped up because they don't have the full context in their head.
blacksmith_tb 5y ago
Right, I think the title should be something closer to "Don't point out something wrong to make yourself feel valuable/smart/etc". Obviously in lots of contexts you do need point it out immediately (the server is down, your neighbor's house is on fire) but when it's just giving feedback, it's good to be helpful (even then there are times when tough love is required, I don't feel bad about pointing out when a mistake would have terrible consequences if it shipped).
- TheOtherHobbes 5y ago
  I think there are status plays, and there are quality plays. Which is which depends on the culture. The motivation is completely different - one is parasitic and narcissistic, one is constructive - but they can look and feel very similar, especially to those on the receiving end. I strongly suspect orgs tend towards a status culture unless carefully steered towards quality. I haven't worked out what the full secret is, but it may have something to do with strongly valuing and respecting skills and input outside of reviews, so specific criticisms aren't experienced as part of a consistent pattern of devaluation.
jka 5y ago
There's so much nuance in code review communication. The way I'd interpret this article's advice in the context of reviews would be: don't begin adding comments about minor annoyances and details that you've noticed until after you've read through most/all of the changeset and raised any more important structural concerns first.

pimlottc 5y ago

Even with a code review, though, you might want to wait until you’ve digested the rest of the review instead of firing off a comment. Maybe it makes sense in context. Or maybe there are bigger fish to fry and it’s not worth quibbling over small things.

nomel 5y ago
> Or maybe there are bigger fish to fry and it’s not worth quibbling over small things. I've taken this approach with one of our engineers, and now we have a huge pile of "small things" that has created some pretty serious technical debt. My perception is that they don't try to understand what I'm pointing out, and come up with rational for how they have it. I think I have a lot to learn.
- jka 5y ago
  I'm still learning too, but I think that something I'm trying to develop is a sense for the longer-term costs and implications of apparent defects. - Is this a component that every engineer is going to interact with and that will stay with the company for decades? - Can this technical decision be reversed easily? Either in the sense of an individual commit revert, and/or in terms of a series of code changes? - Is this change likely to remain isolated to this part of the codebase, or will the pattern infect or be copied to other locations widely? It's super challenging -- and also intellectually rewarding when you can master it and gather team consensus and alignment.
  - hinkley 5y ago
    > Can this technical decision be reversed easily? One of the most exhausting failure modes that I've had to deal with is deciding not to decide. Some people will build elaborate Rube Goldberg machines so that they "can change their minds later" but in fact what they've done is decided that choices are now everybody else's problems. There's a whole hell of a lot of rewards to having the maturity to be able to say "I was wrong" and move on. Writing code in a way that you could rework it to use a different library to accomplish the same thing is making a reversible decision. Creating a rules engine to let a config file make the choices and leaving a cartesian product of states (75% of which are invariably illegal, which you are just supposed to know) is definitely, definitely not. Just pick something, man.
- jrockway 5y ago
  I definitely have a list of things I've let go just to be nice that have caused production outages, consistent poor performance, time-consuming technical debt, etc. It's great to have a good relationship with your team, but at the end of the day, the things that you "let go" can become reasons not to buy your product or why new engineers won't be productive on the codebase. It's very easy to be the person that never says "no" to something, but at the same time, it's also easy to be the reviewer that doesn't advance the reviewee's career. A reviewer should ask themselves; "by not commenting on this, am I making the reviewee a better programmer?" Something I've noticed is that acquaintances will be nice to you universally, but actual friends know when to broach subjects that may not be considered "pleasant" or "nice". Don't give your friends "acquaintance-level" code reviews.
- no_wizard 5y ago
  I have found personally only pair programming with people regularly is the only way you can avoid adding lots of technical debt. Technical debt isn't always avoidable, but I sure know the difference from when teams are heavily invested in pair programming vs not. Good pair programming is hard to ingrain in a culture though.
- babyshake 5y ago
  It is tough to balance between sweating the details and not bikeshedding (sweating the details at the expense of the big important architectural decisions)
- hinkley 5y ago
  I think it's a matter of picking a few things you're going to comment on, and any time the person actually stops needing to be corrected on one of them, you add one of the others in. Any good teacher or mentor does this. But it's a balance because you need the student to see that the reason you're complaining about something new is because they have in fact progressed, and we have moved on to the next thing to address (which has probably always been there)
mwcampbell 5y ago
> Or maybe there are bigger fish to fry and it’s not worth quibbling over small things. This was my excuse to not be thorough when giving feedback in code review, when I was last in a position to do so, while on the Windows accessibility team at Microsoft. I told myself I was on a mission to make Windows more accessible, and there was no time to delay new features or fixes over inconsequential things like coding style or even code organization, as long as it worked. Edit: At least I didn't use that excuse when responding to coworkers' reviews of my PRs.
renewiltord 5y ago
This is a tooling problem. Github reviews vs. Bitbucket reviews will show this. In Github you can write your comments and send them all at once. Until you submit the review, you can just edit your comments and undo them or whatever.
Lockyy 5y ago
This is why I am so grateful for GitHub reviews; Being able to group comments to fire off all at once has saved me from this multiple times as I continue reading, realise something, and remove a previous comment. In fact you reminded me of the technique right now to check the responses and make sure nobody else had already said this!
- spockz 5y ago
  On the other hand, someone coming after you might read the code in the same order and have the same wtf moment. They may even never find that later piece you saw in the review. So at the least it should still be a signal to the Author that it appears strange.
  - funkymike 5y ago
    I also really like this feature of GitHub reviews and when deleting a comment as I continue reading I try to question whether to add a code comment so the next person doesn't get tripped up because they don't have the full context in their head.

blacksmith_tb 5y ago

Right, I think the title should be something closer to "Don't point out something wrong to make yourself feel valuable/smart/etc". Obviously in lots of contexts you do need point it out immediately (the server is down, your neighbor's house is on fire) but when it's just giving feedback, it's good to be helpful (even then there are times when tough love is required, I don't feel bad about pointing out when a mistake would have terrible consequences if it shipped).

TheOtherHobbes 5y ago
I think there are status plays, and there are quality plays. Which is which depends on the culture. The motivation is completely different - one is parasitic and narcissistic, one is constructive - but they can look and feel very similar, especially to those on the receiving end. I strongly suspect orgs tend towards a status culture unless carefully steered towards quality. I haven't worked out what the full secret is, but it may have something to do with strongly valuing and respecting skills and input outside of reviews, so specific criticisms aren't experienced as part of a consistent pattern of devaluation.

jka 5y ago

There's so much nuance in code review communication. The way I'd interpret this article's advice in the context of reviews would be: don't begin adding comments about minor annoyances and details that you've noticed until after you've read through most/all of the changeset and raised any more important structural concerns first.