3 ms·

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

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.

testesttest 6y ago
Write software for another 10 years and I bet your opinion will switch. Nits don't produce substantial value to the business or the code base. Add some decent automated tooling and be done with it. That being said, I would have agreed with you years ago.
- tharkun__ 6y ago
  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.
- dozzman 6y ago
  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.
- bluefirebrand 6y ago
  If a team has a set of coding standards and for some reason does not have automated linting tools to enforce them, then nitpicks in code review are the only way to really enforce the standards. 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.
anothernewdude 6y ago
I have two major rules that address these: 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.

testesttest 6y ago

Write software for another 10 years and I bet your opinion will switch. Nits don't produce substantial value to the business or the code base. Add some decent automated tooling and be done with it. That being said, I would have agreed with you years ago.

tharkun__ 6y ago
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.
dozzman 6y ago
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.
bluefirebrand 6y ago
If a team has a set of coding standards and for some reason does not have automated linting tools to enforce them, then nitpicks in code review are the only way to really enforce the standards. 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.

anothernewdude 6y ago

I have two major rules that address these: 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.