I'm not sure how much I agree. The author says there are 2 big benefits:
1. Vastly improved signal-to-noise ratio
2. Better relationship with code reviews
For 1, in most CR systems you can mark the comments as resolved once their fixed. They don't even have to show up once they're fixed. That should improve the signal-to-noise ratio. In addition, nobody should really just be glossing over any comment, regardless if it's a nit or if it's a serious change.
For 2, I still think that you should try to not take code reviews personally. Perhaps it's more a sign of the environment or the company these CRs are happening in? That "zen-like acceptance" isn't something that just pops out of nowhere, it's something that has to be developed and built. Strangely, when I get a bunch of nits on my CR, it actually generally makes me happy. It's proof that the other developers on my team couldn't really find anything wrong with the code I wrote.
Of course, I'd still love to have 0 comments instead of a bunch of nit comments, so I 1000% agree with using automated tools to lint code, ideally in multiple places (before build, before commit, after commit, before merge, after merge, ...). I think it also helps focus reviewers on giving more meaningful feedback.
I would like to provide 'the middle ground' so to speak :)
I think it depends on the 'nits' and the environment. "pointing out a variable name that could use a more appropriate word" as the article puts it, is not a nit if you ask me. It's something that is just part of Clean Code. Now the way that you message that you found such a suboptimal variable name plays a big role I believe as does the overall culture in the company and in the PRs. It will also depend on your relationship history with the guy pointing it out and whether you've worked with him in person/close contact or not. Friends can 'play rough' easily (what to an outsider might look like you're fighting) while actually having fun with each other. The exact same behaviour/communication would seriously turn off someone you don't really know very well.
At my place for example, we collectively have a high standard on these things and it's absolutely not considered nitpicking to point out these things. You're generally grateful for them as it happens to all of us that we just use 'some name for now' while trying out an idea and we overlook one while 'cleaning up'.
In the same vein, the "boy scout rule of coding" - "Always leave the code base in a better state than what you found it in" - can either be applied in a good way or can be taken way too far. It's all about the balance. Just like in the real camp ground, sure you will pick up the trash you found in the fire pit and when you find some more trash while wandering about camp, you'll pick that up too and throw it out. But you're not gonna laboriously interrogate every square inch of the camp ground plus a two mile radius around it, are you? You'll never get anything done that way.
I also would have agreed with OP 5 years ago. Working in a leadership capacity for a prolonged period has emphasised the human side of coding, that is working in a team and respecting your colleagues’ freedom to think independently. IMO it’s far more important to maintain a strong and happy-to-work team than a marginally more aligned-with-my-own-thinking codebase.
The first is that code review comments are leaping off points for discussion, and filling in the space next to the comment with a justification is a valid fix. (And "I don't understand this" is a valid comment) This helps equalise the two roles, and makes it just as valid for a junior to review a senior's code, and avoids artificially limiting who can review any piece of code.
The second is that everybody reviews everybody else's code regularly. The source of people taking them personally (in my experience) are people who haven't had their code reviewed in a while or having the same people do all the reviewing.
With the right definition of nit, I agree. However, the first example is "a variable name that could use a more appropriate word" AKA "a variable name that increased confusion". If perfect performance optimization doesn't matter, then don't review for it, but code readability does always matter. It won't be them who has to fix it in a few years, so take every opportunity to change things that put friction on the path of enlightenment to within the bounds of your capacity to sacrifice optimizations.
It doesn't make you an irascible ass to say "I find this variable name confusing. Can we change it to something more like X?"
> "I find this variable name confusing. Can we change it to something more like X?"
My problem with stating it like this is that when the person saying this is in a position of authority it’s extremely hard to say no to it. And that person might not always be necessary right, or the person writing the code had a good reason for this naming, and now can’t argue against it.
While there are best practices for naming, people often have different opinions, but that doesn’t mean one is right and the other one wrong.
Good. A person just told you that you did something that confused them. You don't have to rename it to "X" exactly, but you do need to try to do something that communicates effectively to your collaborators.
> And that person might not always be necessary right
There is no right. There are only confusing and not-confusing. A person that you're collaborating with just told you that what you did is unclear to them. That means you have to make things more clear. I don't care if your solution to this problem is to just add a comment, as long as you're both happy that the end result communicates your ideas effectively. But you have to do something to fix the fact that your original variable name didn't communicate what you thought it would.
> that doesn’t mean one is right and the other one wrong
Thinking of names as objectively right or wrong is a mistake. Names either add confusion or reduce confusion. Striving for the latter requires meeting your interlocutor halfway.
> A person just told you that you did something confusing.
The person told me that I did something confusing to them. And with using that kind of format, almost forces you to have to pick his opinion. Because now instead of clarifying your choice of naming, have to now defend your choice against his or her.
> There is no right/wrong here. There is only confusing and not-confusing. A person that you're collaborating with just told you that what you did is _unclear_ to them. That means you have to clarify what you did.
A person just told me they find a name confusing, and asked if it can be changed. That person could’ve clarified why it was named a certain way, but instead just proposed a change.
Again, the way the comment was written makes it extremely hard to do anything other than use the proposed new name.
> Thinking of names as right or wrong is a mistake. Names either add confusion or reduce confusion. Always strive for the latter.
That’s an extremely subjective statement, and to me is not how things are in the real world.
> And with using that kind of format, almost forces you to have to pick his opinion.
I said that it needs to communicate the right information to both of you, not just to you and not just to them. If their suggestion still communicates to you, then suck it up and admit that they came up with a better name. If it _doesn't_ but you use their suggestion anyway, then you've just failed at being part of a team. It's on you to learn how to say "that's unclear to me, let's look for a third option".
> have to now defend your choice
It sounds like you think you're likely to feel defensive about having created confusion for someone. Maybe focus less on defending and more on achieving clarity.
> I said that it needs to communicate the right information to both of you, not just to you and not just to them.
Right, but you see how it took multiple back and forwards to even clarify what you wanted to achieve?
The same goes with your original comment.
> "I find this variable name confusing. Can we change it to something more like X?"
You don't clarify why it's confusing, so I don't know your interpretation of what the variable represents. I need to first understand as to why you find this confusing, and how your new proposed naming will help to clarify.
> It sounds like you think you're likely to feel defensive about having created confusion for someone. Maybe focus less on defending and more on achieving clarity.
I do not, and it's pretty bad practice to assume this of people. This comes back to the whole nitpicking, to me it sounds like you were nitpicking on the naming. You didn't clarify your stance, or what caused you confusion. And leave it up to me to reach out to you to clarify this. This could've been communicated in your first PR comment, so that we didn't have to go through this dance.
> Right, but you see how it took multiple back and forwards to even clarify what you wanted to achieve?
Luckily we're in this together having a conversation with an interest in achieving mutual clarity.
> I need to first
That's ok! You should feel comfortable expressing your needs. That's the nature of discourse. I guess if I were to turn your argument back around on you I would say "you could have expressed your need from the start."
But nobody but me is inside my head and nobody but you is inside your head, and neither of us knows what the other needs until we tell the other. So we do our best to get there eventually. I tell you that your chosen name doesn't make sense to me and propose an alternative that does. Maybe you say "ok, that works for me too", or maybe you say "that doesn't work for me but if you tell me more about the problem maybe there's another alternative." That's how teamwork goes.
But if my alternative suggestion _does_ also communicate to the other person but they demand that I convince them anyway because they want to be "right", well, then that person is just being hostile and defensive, which makes being on a team more difficult.
> I tell you I need a different name. You tell me that knowing why will help you come up with solutions. That's how teamwork goes.
I totally agree with that. And if this was happening quite often, I would somehow propose a better process for these kind of naming changes. Communication is always key, especially when it comes to written text.
> I guess if I were to turn your argument back around on you I would say "you could have expressed your need from the start."
That's kind of my point, if you had clarified your stance in your original comment, then we wouldn't have to go through this and instead could've focused on coming up with an appropriate name. In that sense I can agree with the OP on why this could be considered nitpicking.
By definition "nitpicking" is not desirable. The problem is what is the magnitude of improvement that bridges the gap between a nit and a substantive improvement?
When I started off as a code reviewer, I would frequently comment when "I would have done it differently". Now I make sure I only comment when "I would have done it better" and I always explain why it is better. Writing that explanation forces me to ask myself, "is this better?" and I also really want it to be substantially better.
There are a category of changes I request which I think of as "nits". Usually these are cosmetic changes that I think genuinely improve the code quality. I think they are worth asking why should we have a misspelling in a documentation string when we could not have it? It's just going to annoy everyone who reads that code until some unrelated change decides to fix it. When I have these kinds of comments, I preface them with "nit:".
I do this at work, even with newer developers, although I try give them more slack and time to adapt to our commit & PR style. Do I need to pick on minor formatting issues or a spelling mistake? Do I want to? No. My concern is that the long term cost of maintaining and fixing this when it was actually cought in PR.
So whenever someone nitpicks on my code it's usually for the better, i.e changing a method name.
I recently realized that I was spending more time telling people what I wanted them to do in their merge requests than I would spend just merging their thing and patching it the way I want.
So, for small projects I manage on my own, this totally makes sense: instead of saying "this comment has a typo" or "could you rephrase this message", I find that it's easier to just rephrase the thing myself than do another round-trip with the contributor.
It makes everyone happier: I get things exactly the way I want them, and the contributor doesn't feel like they're wasting time on trivialities, while still getting their contribution acknowledged.
The "suggest change" feature in GitHub and GitLab does do some of that, by the way, so for me that's a viable alternative.
And of course this works only if you're the sole (or one of few) maintainers who can commit directly to the HEAD branch. If you need to do another merge request to fix those issues, that process doesn't work so well, obviously, and there are some errors that just need to be fixed.
But I found that it pays to be less "nitpicky". It certainly makes for a more relaxed ambiance. :)
It's neither possible nor desirable to reprogram nitpickers, many of whose obsessive compulsive tendencies are precisely why they're good programmers. That's why I institute the policy that reviewers must counter-submit a PR based on the original PR's commits. You want the change, you make it yourself.
The code review software I use at work lets reviewers add suggested edits that can be applied with a single click by the author of the commit. I try to use this as often as I can and mark any nits as non-blocking so they can still submit without another round trip if they decide against fixing for whatever reason.
I think nits are valuable but are absolutely not the focus of code review. Personally I'd rather see a few small nits on review of my code than just approval because that suggests the reviewer engaged with the code for more than just a quick glance.
The "single click applied suggestion" thing of course doesn't work if you have a policy of avoiding trivial commits (actually a very good policy) and your PR is already about 3 or 4 commits. Then the author has to sit down, figure out which commit a change belongs to & do a `rebase -i` to squash together associated commits...
As time has gone on I’ve become a bit better about being too particular. I still sometimes point out nit picks, but critically I ask myself if I could live with what was there. If I could, then I put my approval on it and move on. If it’s something more critical, then I wait for a response, fix, or discussion about it. Seems to make the reviews easier and more worthwhile.
I also think it's important to think about the other side of this, which is why nitpicking happens in the first place. In my experience, the underlying reasons why people nitpick code have little to do with shipping the best possible version of the product --- rather, nitpicking is a response to:
1) pressure from PMs/POs for quick, yet "effective" code reviews that don't hold up releases, and
2) implicit pressure from other developers to maintain "polite" relations by only pointing out minor, "typo"-level issues with their code instead of requesting architectural changes that could be seen as condemning their work.
#2 there sounds rather terrifying. I'm not sure I understand #1. Is the idea that folk will nitpick in order to appear like their code review was thorough for their managers?
I've mainly worked on safety-critical systems, and thorough code review is generally expected and welcomed in that space. In my experience, perceived nit-picking is rooted in two things. 1. Lack of consistent style guidelines or unwillingness to conform to style guidelines. 2. Arbitrary design decisions that weren't vetted by anyone until code review.
The former can be generally managed by agreeing on expectations outside of individual code reviews, and then referencing the relevant documentation when pointing out style problems. The latter can be managed by encouraging folk to communicate their plan and solicit feedback long before they submit fully baked code for review. Not to say that folk shouldn't just knock out code if they're motivated to, but without earlier communication they must be willing to defend their code in review.
As a reviewer, it also helps to annotate comments. If you feel like a comment might be a nit or might be perceived as a nit, just call it what it is i.e. "nit: this variable name could be better". At the end of the review, if it's all just nits, give your coworker an "out" by saying, "looks great! There's just a bunch of nitpicks, so feel free to address how you see fit and ship it." Or if it's more than a nit but not a bug, "Overall this looks pretty good. There's one thing there with a potential divide by zero if the config file is bad, but if you're in a hurry I'd be happy with a TODO comment".
To reiterate, managing expectations is important for avoiding code review drama. Typically with new hires, their first few PRs I review often end up with upwards of 50+ comments, most of them style related. The first thing I do before even looking at the code, though, is send them a message saying that I am a thorough code reviewer, I'm not trying to give them a hard time, and their first few PRs are likely going to be a bloodbath since they're new to the code base. Afterward, most folk usually go out of their way to thank me for being upfront about it, and that they really appreciated the thorough feedback.