Stop Nitpicking in Code Reviews
blog.danlew.net
blog.danlew.net
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.
That being said, I would have agreed with you years ago.
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.
Overall, this makes the codebase much easier for the whole team to work on. That's why we make code standards in the first place.
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.
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?"
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.
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.
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.
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.
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 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.
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:".
So whenever someone nitpicks on my code it's usually for the better, i.e changing a method name.
I started working around 5 years ago in a startup. I was a bloody beginner and we had someone joining who was pretty senior, who I suspect had OCD to some degree.
Being a startup we needed to ship, quickly.
That senior developer joining was working quite differently. He did not believe in auto-formatting, but he cared a lot about things which did not degrade the end product by the least bit.
E.g.: He had this super specific way of ordering CSS properties. It kinda made sense but I had to look it up every time and we did not have a linting rule for it.
He kept blocking PRs due to this, or a tiny typo in some comment. Being British he also kept correcting grammar.
He regularly found some extra breaking space or non-breaking space which was off. It meant that the PR is not gonna go through, until resolved.
Often, my PRs comment count would shoot past 50, at times even hundred, and I was becoming more and more desperate.
Today I tend to say: if it’s not lintable, it’s not a codestyle rule, and it’s been pretty great ever since.
Being British. I feel personally attacked.
I assure you, his nationality had no correlation to his banal ways. Unless he was trying to correct your spelling of organization to organisatio which I can forgive.
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. :)
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.
If the edit is an easy bug-fix, the author should just look over it for a sec and approve. If it's a style change in some cases you don't even need the author to look at it.
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.
(And that's not to say it's not sometimes useful to ask someone to fix something else they noticed, but to nitpick on it is the worst.)
Getting good at code review is about learning that just because something isn't exactly how you'd do it, byte-for-byte, doesn't mean it's not "fine".
I even wrote a custom vscode plugin to encourage devs to do just that: https://marketplace.visualstudio.com/items?itemName=checkr.c...
Now it only takes me 10-30 minutes to whip up a new lint check.
Having a common linter saves a lot of trouble.
For everything else, I explicitly "prefer but don't insist". It's up to my colleagues to decide if the feedback is important.
Talking over the plan in advance is much more efficient than after the fact code review.
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.
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.