5 ms·

> The Priority Inversion > In your first code review passes, pick small and simple nits. Variable names are a bit unclear; comments have typos in. > Wait for

> The Priority Inversion > In your first code review passes, pick small and simple nits. Variable names are a bit unclear; comments have typos in. > Wait for the developer to fix those, and then drop your bombshell: there’s a much more fundamental problem with the patch, that needs a chunk of it to be completely rewritten – which means throwing away a lot of the nitpick fixes you’ve already made the developer do in that part of the patch. I've done this more than once, and felt pretty bad about it each time. The problem is that I'm pretty good at spotting the little issues while I'm reading through some code. But I'm not as good at identifying the larger design issues until I'm thinking about it later

progbits 2y ago
I had a coworker who would leave nitpicks and bombshell in the same review round. Like 30 comments, 29 of which are irrelevant if you read the last one. Couldn't explain the problem to him... I just learned to read through full review from him first before starting to address anything. Otherwise he was one of the best people I worked with and the feedback was valuable.
- operator-name 2y ago
  In my mind it's important to have both, as long ad the main issue is made clear and nitpicks are truly nitpicks (and thus acceptable in limited quantity to not block a change). Otherwise you'd have a "bombshell" to refactoring, then on the 2nd review pass a handful of nitpicks that you could have addressed during the initial refactor.
- sqeaky 2y ago
  What really is the alternative? Just post the big bombshell thing and not express your opinion about stuff? Often it's the expressing opinion part that's more important than the actual nits.
  - progbits 2y ago
    It was like: > This whole thing won't work because of X and you need to pretty much rewrite all of this. > > Rename foo to bar. > Typo. > Missing whitespace. So yeah... Just don't post the rest? I often go back to earlier comment drafts and edit / delete them when I realize deeper into the review they are no longer relevant. The review comments should not be a stream of consciousness but properly written feedback.
    - aidos 2y ago
      > The review comments should not be a stream of consciousness but properly written feedback. My advice would be to not take it so personally and assume good intentions from your reviewers. If someone gave me feedback that caused a big course correction then I’d slap my forehead for missing it, thank them, then move on with my life. The extra feedback of other issues is a bonus. Asking them to spend more time reviewing their review is wasted effort.
    - deleted 2y ago
      [deleted]
  - dontlikeyoueith 2y ago
    > Just post the big bombshell thing and not express your opinion about stuff? Yes. > Often it's the expressing opinion part that's more important than the actual nits. Your opinions are not that valuable. Focus on the most valuable feedback you can give first.
    - wakawaka28 2y ago
      If you take the time to spell out generic complaints, it feels bad to throw that away. I think that a good compromise is to have a system that allows a summary of the review to be posted with the comments, so you can mention the important thing up front.
      - sqeaky 2y ago
        It isn't even about feeling bad, it's that if they write it they're going to make the same mistakes again in a little bits. If someone misspells a word habitually and you think maybe it should be spelled correctly (or whatever fix) then maybe that needs to get pointed out needs to be taken into account when they do some big rewrite.
      - dontlikeyoueith 2y ago
        [flagged]
- manmal 2y ago
  The review description (at least GitHub has this) should contain a TLDR.
- rileymat2 2y ago
  Is this an artifact of the tool? Promoting leaving notes as you go, for him only to find it later, with no way in the tool to reprioritize?
  - progbits 2y ago
    The review tool kept drafts and had a separate "submit review" step. Very easy to edit / remove comments. Most people did, he did not.
ysavir 2y ago
This is why I like Github's feature to add comments before posting the full review. You can do a round of nitpick comments, take some time away to let the rest settle, come back and re-review with the big picture. You can then add those comments as well, remove some of the nitpicks if they aren't relevant/important enough anymore, etc.
- TeeMassive 2y ago
  Bitbucket(for those of us who are stuck with Atlassian) finally has this feature now.
wk_end 2y ago
No one's perfect; it's OK to make mistakes, so long as you identify that it's a mistake to yourself, and apologize for your mistake to others. It's only when a reviewer does this (or the various other sins listed here, or a few more besides) without acknowledging it/displaying empathy/working to help unblock that I start giving my resume a refresh.
eikenberry 2y ago
I find nitpicks to not only be useless to the author, but to actively add noise that either masks any real issues you want noted or just give the author meaningless work. They seem more like a tonic for the reviewer to be able to say something to indicate they did look at the code and just result in wasted time.
- shepherdjerred 2y ago
  It depends on the nitpick. If it's something like a spelling error, then that's worth fixing. If it's formatting or something that can be linted, then that should be automated. If it's code style, I generally let that slide if it's just a difference in opinion, or I'll try to have a discussion with my coworker about our preferences (though I would still approve the PR). If it's genuinely bad code or a junior dev then it makes sense to provide feedback.
  - eikenberry 2y ago
    > If it's something like a spelling error, then that's worth fixing. My take on this is that if it's worth fixing it's not a nitpick. Small problems are still problems. Automated stuff, if not automated, is also an issue that should be fixed. Teaching opportunities are also not nitpicks, as teaching is important. For me the acid test for a nitpick is that if I wouldn't add a commit just to fix it, it is not worth a comment. I'd add a commit to fix a spelling error, but not to tweak some variable name. This is assuming a real commit and not something you're planning on rebasing away.
    - shepherdjerred 2y ago
      If you define "nitpick" as "something that's not worth fixing" then by definition nitpicks are worthless. I've heard the term nitpick usually used to mean "small comment", "my preference", or "very minor". > Teaching opportunities are also not nitpicks, as teaching is important. Preference usually isn't a teaching opportunity, it's just a minor disagreement. It's trying to make your code look like I (the reviewer) wrote it. For example maybe you like more terse names than I do. We can argue about every point of personal preference, but that's not a great use of our time.
throwuxiytayq 2y ago
To be fair, it’s often difficult to identify the important problems when the code is very messy. I don’t know if that’s just my hole-ridden brain, but sometimes after I clean up a piece of code it unlocks a new level of understanding and lets me spot issues that seemed completely invisible beforehand. It’s why when programming I put a lot of effort into code formatting and structure (eg. function extraction and inlining, and other transformations that don’t alter behavior).

progbits 2y ago

I had a coworker who would leave nitpicks and bombshell in the same review round. Like 30 comments, 29 of which are irrelevant if you read the last one. Couldn't explain the problem to him... I just learned to read through full review from him first before starting to address anything. Otherwise he was one of the best people I worked with and the feedback was valuable.

operator-name 2y ago
In my mind it's important to have both, as long ad the main issue is made clear and nitpicks are truly nitpicks (and thus acceptable in limited quantity to not block a change). Otherwise you'd have a "bombshell" to refactoring, then on the 2nd review pass a handful of nitpicks that you could have addressed during the initial refactor.
sqeaky 2y ago
What really is the alternative? Just post the big bombshell thing and not express your opinion about stuff? Often it's the expressing opinion part that's more important than the actual nits.
- progbits 2y ago
  It was like: > This whole thing won't work because of X and you need to pretty much rewrite all of this. > > Rename foo to bar. > Typo. > Missing whitespace. So yeah... Just don't post the rest? I often go back to earlier comment drafts and edit / delete them when I realize deeper into the review they are no longer relevant. The review comments should not be a stream of consciousness but properly written feedback.
  - aidos 2y ago
    > The review comments should not be a stream of consciousness but properly written feedback. My advice would be to not take it so personally and assume good intentions from your reviewers. If someone gave me feedback that caused a big course correction then I’d slap my forehead for missing it, thank them, then move on with my life. The extra feedback of other issues is a bonus. Asking them to spend more time reviewing their review is wasted effort.
  - deleted 2y ago
    [deleted]
- dontlikeyoueith 2y ago
  > Just post the big bombshell thing and not express your opinion about stuff? Yes. > Often it's the expressing opinion part that's more important than the actual nits. Your opinions are not that valuable. Focus on the most valuable feedback you can give first.
  - wakawaka28 2y ago
    If you take the time to spell out generic complaints, it feels bad to throw that away. I think that a good compromise is to have a system that allows a summary of the review to be posted with the comments, so you can mention the important thing up front.
    - sqeaky 2y ago
      It isn't even about feeling bad, it's that if they write it they're going to make the same mistakes again in a little bits. If someone misspells a word habitually and you think maybe it should be spelled correctly (or whatever fix) then maybe that needs to get pointed out needs to be taken into account when they do some big rewrite.
    - dontlikeyoueith 2y ago
      [flagged]
manmal 2y ago
The review description (at least GitHub has this) should contain a TLDR.
rileymat2 2y ago
Is this an artifact of the tool? Promoting leaving notes as you go, for him only to find it later, with no way in the tool to reprioritize?
- progbits 2y ago
  The review tool kept drafts and had a separate "submit review" step. Very easy to edit / remove comments. Most people did, he did not.

ysavir 2y ago

This is why I like Github's feature to add comments before posting the full review. You can do a round of nitpick comments, take some time away to let the rest settle, come back and re-review with the big picture. You can then add those comments as well, remove some of the nitpicks if they aren't relevant/important enough anymore, etc.

TeeMassive 2y ago
Bitbucket(for those of us who are stuck with Atlassian) finally has this feature now.

wk_end 2y ago

No one's perfect; it's OK to make mistakes, so long as you identify that it's a mistake to yourself, and apologize for your mistake to others. It's only when a reviewer does this (or the various other sins listed here, or a few more besides) without acknowledging it/displaying empathy/working to help unblock that I start giving my resume a refresh.

eikenberry 2y ago

I find nitpicks to not only be useless to the author, but to actively add noise that either masks any real issues you want noted or just give the author meaningless work. They seem more like a tonic for the reviewer to be able to say something to indicate they did look at the code and just result in wasted time.

shepherdjerred 2y ago
It depends on the nitpick. If it's something like a spelling error, then that's worth fixing. If it's formatting or something that can be linted, then that should be automated. If it's code style, I generally let that slide if it's just a difference in opinion, or I'll try to have a discussion with my coworker about our preferences (though I would still approve the PR). If it's genuinely bad code or a junior dev then it makes sense to provide feedback.
- eikenberry 2y ago
  > If it's something like a spelling error, then that's worth fixing. My take on this is that if it's worth fixing it's not a nitpick. Small problems are still problems. Automated stuff, if not automated, is also an issue that should be fixed. Teaching opportunities are also not nitpicks, as teaching is important. For me the acid test for a nitpick is that if I wouldn't add a commit just to fix it, it is not worth a comment. I'd add a commit to fix a spelling error, but not to tweak some variable name. This is assuming a real commit and not something you're planning on rebasing away.
  - shepherdjerred 2y ago
    If you define "nitpick" as "something that's not worth fixing" then by definition nitpicks are worthless. I've heard the term nitpick usually used to mean "small comment", "my preference", or "very minor". > Teaching opportunities are also not nitpicks, as teaching is important. Preference usually isn't a teaching opportunity, it's just a minor disagreement. It's trying to make your code look like I (the reviewer) wrote it. For example maybe you like more terse names than I do. We can argue about every point of personal preference, but that's not a great use of our time.

throwuxiytayq 2y ago

To be fair, it’s often difficult to identify the important problems when the code is very messy. I don’t know if that’s just my hole-ridden brain, but sometimes after I clean up a piece of code it unlocks a new level of understanding and lets me spot issues that seemed completely invisible beforehand. It’s why when programming I put a lot of effort into code formatting and structure (eg. function extraction and inlining, and other transformations that don’t alter behavior).