5 ms·

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

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.

sgtnoodle 6y ago
#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.

sgtnoodle 6y ago

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