Hacker News Reader

Comment by sharkjacobs | Hacker News Reader

> 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

View on HN

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.

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

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).

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.

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.

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.

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.

> 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.

> 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.

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.

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.

The review description (at least GitHub has this) should contain a TLDR.

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?

The review tool kept drafts and had a separate "submit review" step. Very easy to edit / remove comments. Most people did, he did not.

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.

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.

> 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.

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.

> 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".

Those are all things that are not worth fixing/changing and so that is just a different way of restating it. I.E. "small comments", "my preference", "very minor" things are all worthless in a PR. They might be interesting when hanging out and chatting, but for a productive workflow they are just noise.

Same with preferences... if you are just relying a preference, not worth it. If you are teaching a better idiom or similar, then it might be. This is not hard and fast, just that in general most "nitpick" types of PR comments are a net loss.

This is why I love Conventional Comments: https://conventionalcomments.org/

Nitpicks are explicitly declared as such and since everyone participating the PR know that they aren't meant to be blocking at all and are encouraged to ignore them if they take too much time than they are worth.

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.

Reply on news.ycombinator.com