5 ms·

> Code reviews have become a social thing, and bagging on someone with a litany of issues in front of a group is never acceptable behavior. If they've done ten

> Code reviews have become a social thing, and bagging on someone with a litany of issues in front of a group is never acceptable behavior. If they've done ten things wrong, you have to pick 3 of them and let the rest go, or decide to kick them out of the group entirely. I wonder if we overestimate how sensitive people are in this case. if someone raises ten objections on my review and makes it clear what I have to do to get their approval, I'm happy. I'll get right to work on fixing it. if this happens in front of a group (esp involving skip-levels / managers of other teams), I take it as a signal that I should have run the change by a teammate or my direct manager before involving so many people. the only thing I actually dislike is when people comment about vague concerns that leave it unclear whether I actually have to address them in that change to get their approval. sometimes this causes a circular waiting deadlock with the other reviewers, which is especially frustrating.

wccrawford 6y ago
It depends on both the person and the environment. In an environment that places stress on those who underperform, anything that calls attention to performance problems will be hated. Anyone that points out those problems will be disliked. Even the best people will fall to this stress. In a more relaxed environment that isn't constantly counting points towards raises and promotions, people can relax and not worry about the problems that are raised. However, not everyone can manage this. Some people have just had too many jobs of the previous type, or other experiences like that. They just can't adjust. IMO, it also means they have a much harder time fixing their own problems because they can't tolerate other people pointing out those problems. As for code reviews, I always call it like I see it. It's my job to make sure the code will work and be secure. I'm not going to let things slip by that violate that, no matter how much I want that person to be happy. And I'll still point out matters of style as well, though I usually won't insist they be corrected, unless the person has repeatedly had the same problem. (My people are generally great, though, and will fix even my suggestions. So it's generally not a thing.) You'd probably hate that last bit of my code reviews. If we were on a team, my advice would be to simply discuss that part with me in real time. We could decide together if it was worth the effort to make those changes. Async communications like PRs and emails simply aren't fit for that kind of discussion.
- hinkley 6y ago
  It also depends if everyone else pointed out 2 things and you pointed out 10. Letting other people go first means you only have to point out 5, which is still lopsided but not 'holy shit, man' lopsided.
- nicoburns 6y ago
  > In an environment that places stress on those who underperform, anything that calls attention to performance problems will be hated. Anyone that points out those problems will be disliked. Even the best people will fall to this stress. Seems to me that this might be the problem rather than the code review itself! > We could decide together if it was worth the effort to make those changes. Async communications like PRs and emails simply aren't fit for that kind of discussion. Indeed. I've definitely found myself falling into a pattern of failing the review for serious issues with comments on the PR, but just approving the review and slacking the comments for small issues.
  - Person5478 6y ago
    This is probably my biggest beef with the "culture" of software development nowadays. Back in my day (hold on while I go grab my cane...) software developers would just flap their lips and sound would come out at each other. Nowadays there seems to be this underlying assumption that actually talking to each other is a waste of time, that it should all be documented with no curation, and should always be in PR's, et al. I'm all for documenting decisions, but I feel like it's not too much effort to swing by someone's office and have a discussion with them. At that point, maybe you don't even need to document if the updates are small issues. Is it really so all-encompassingly important? But maybe I'm just old. Most of the time I'd rather talk to a human when I call into tech support too (unless it's something obviously automated like making a payment).
    - nicoburns 6y ago
      > I feel like it's not too much effort to swing by someone's office and have a discussion with them Well there is a modern trend that's supposed to help with this too: open plan offices. No need to swing by their office, they're already right there (this has indeed been my working environment for most of my career). It's all a bit different at the moment of course, but my team regularly video calls each other to discuss things that would take too long over text.

wccrawford 6y ago

It depends on both the person and the environment. In an environment that places stress on those who underperform, anything that calls attention to performance problems will be hated. Anyone that points out those problems will be disliked. Even the best people will fall to this stress. In a more relaxed environment that isn't constantly counting points towards raises and promotions, people can relax and not worry about the problems that are raised. However, not everyone can manage this. Some people have just had too many jobs of the previous type, or other experiences like that. They just can't adjust. IMO, it also means they have a much harder time fixing their own problems because they can't tolerate other people pointing out those problems. As for code reviews, I always call it like I see it. It's my job to make sure the code will work and be secure. I'm not going to let things slip by that violate that, no matter how much I want that person to be happy. And I'll still point out matters of style as well, though I usually won't insist they be corrected, unless the person has repeatedly had the same problem. (My people are generally great, though, and will fix even my suggestions. So it's generally not a thing.) You'd probably hate that last bit of my code reviews. If we were on a team, my advice would be to simply discuss that part with me in real time. We could decide together if it was worth the effort to make those changes. Async communications like PRs and emails simply aren't fit for that kind of discussion.

hinkley 6y ago
It also depends if everyone else pointed out 2 things and you pointed out 10. Letting other people go first means you only have to point out 5, which is still lopsided but not 'holy shit, man' lopsided.
nicoburns 6y ago
> In an environment that places stress on those who underperform, anything that calls attention to performance problems will be hated. Anyone that points out those problems will be disliked. Even the best people will fall to this stress. Seems to me that this might be the problem rather than the code review itself! > We could decide together if it was worth the effort to make those changes. Async communications like PRs and emails simply aren't fit for that kind of discussion. Indeed. I've definitely found myself falling into a pattern of failing the review for serious issues with comments on the PR, but just approving the review and slacking the comments for small issues.
- Person5478 6y ago
  This is probably my biggest beef with the "culture" of software development nowadays. Back in my day (hold on while I go grab my cane...) software developers would just flap their lips and sound would come out at each other. Nowadays there seems to be this underlying assumption that actually talking to each other is a waste of time, that it should all be documented with no curation, and should always be in PR's, et al. I'm all for documenting decisions, but I feel like it's not too much effort to swing by someone's office and have a discussion with them. At that point, maybe you don't even need to document if the updates are small issues. Is it really so all-encompassingly important? But maybe I'm just old. Most of the time I'd rather talk to a human when I call into tech support too (unless it's something obviously automated like making a payment).
  - nicoburns 6y ago
    > I feel like it's not too much effort to swing by someone's office and have a discussion with them Well there is a modern trend that's supposed to help with this too: open plan offices. No need to swing by their office, they're already right there (this has indeed been my working environment for most of my career). It's all a bit different at the moment of course, but my team regularly video calls each other to discuss things that would take too long over text.