STOP Nit Picking In Code Reviews

Recorded live on twitch, GET IN

MY MAIN YT CHANNEL: Has well edited engineering videos

Discord

Hey I am sponsored by Turso, an edge database. I think they are pretty neet. Give them a try for free and if you want you can get a decent amount off (the free tier is the best (better than planetscale or any other))

[notuxnobux]

I love when the deadline is today and there is nitpick code reviews that delay the review and merge process by 5 more days

[taylorallred]

I had an engineer say to me once “if I put a nit in a review then it means I don’t really care if you fix it. It’s just an idea” and that’s how I’ve chosen to see nits from now on.

[viniciusleitepereira]

I love to be called a C# loser so early in the morning. Thanks for making my day better, Prime 😊

[Ascentyon]

Regarding tests that don't provide any value... My friend once told me something very interesting: He wished testing frameworks could have a single switch, that flipped all the asserts to be the inverse. Then, all your tests should fail. If any still pass, they can be deleted because they're absolutely useless.

[eloniusz]

You guys discussing about validity of nitpicks and I'm standing in the corner crying because I would like to have this problem. I would like anyone to read my request before approving it. Even if I explicitly ask for thorough review because I have doubts about my changes, they are looking for max 10 second. Their thoughts (probably): "Yes, indeed this seems to be a computer code. APPROVED!"

[henriquesouza]

Most of the time it's not about right or wrong, it's about consistency. If all private variables in the project start with an underscore (_), and someone make a PR with code without the underscore because they don't like it, it's gonna be 'nitpicked' about it, even if they agree with you, because of consistency. If you want to remove the underscore for your private variables, then do so in the entire project, so that it's not inconsistent.
I don't like that the consistency factor hasn't been raised in the article nor in the video. It's literally one of the pillars of programming (imo).

[xpeter]

I love in person peer reviews. Much faster and much less bad vibes about to many comments. Plus you learn so much from the other developer

[stefanms]

The best codebase I ever worked on was one where the lead developer had OCD and would point out every small issue. Nits might be annoying, but they are great for the long term.

[asdfasdf]

Indeed, a matter of noise vs signal: having a zoo of naming conventions, api styles, or, in general, different ways to do same thing, is hella noisy. The trouble of couple extra code review cycles during onboarding (and maybe few "your-convention-is-wrong"-s who fail to grasp the concept of style consistency) is peanuts in comparison to stumbling over every other function for extra minute, trying to figure if it looks different because it does a different thing, or just because we ceased to bother nitpicking some time ago.

[freedom]

I learnt so much from getting my code absolutely bashed by people more experienced than me :)) it’s literally FREE education

[FraggleH]

I always find myself regretting letting something apparently unimportant go in review. It's scary how often it's come back to bite me.

[patrickkhwela]

For me in my company I blame management for this. People are encouraged to add their two cents in a PR just to fell like they are adding some sort of value when they are not.

[raul_ribeiro_bonifacio]

I still nit pick sometimes but I found out that people usually accept it without problems if you name them "suggestions" rather than "fixes to be done". After all they are just another category of issue, if they really are. And there's another issue: if your work environment gets emotionally affected by this often it means the programmers are too sensitive about their code and criticism, and that is not a healthy either. Sensitive programmers call every "criticism" a "nit picking" to justify their poor practices as well... That's why I keep a healthy dose of nit picking and to keep things in balance.

[oShwavyo]

As a more junior dev I love to be nit-picked. How else am I going to learn how people usually do things unless someone tells me? The more experience I gain the less I act on every single nit comment, but at first I was making every single suggested change and asking the commenter for an explanation if I didn’t understand the reason behind the suggestion.

[JoesphGeorgeBlaise]

I like the article, but I do feel like there's an implicit assumption that it's clear what's a nitpick and what isn't... Simple example is that a variable name that's totally wrong is not a nit. A variable name that could be slightly clearer or fit with some slightly arbitrary convention is a nit, but there's a pretty decent grey area in between.

[dealloc]

For us nitpick comments are always prefaced with "nit: " and can be ignored by the PR submitter as they are not important to the overall goal of the implementation. Our nits are mostly just how to simplify something (i.e. make it more readable) or improving type safety, or adding comments for temporary solutions to problems and why they are needed, for example.

We keep them to a maximum of 2-3 nits (depending on size of PR), they have to come with an easy copy-paste suggestion, and we never discuss nit picks, it's either applied it or not. No stream of comments.

[vikramkrishnan]

99% of nitpicks belong to the realm of linters and formatters.

Edit: Commented before watching the full video.

[kipcrossing]

Merge now, fix when a bug is found in prod

[ilearncode]

I once got dev blocked on a massive new feature coming in because the most senior dev (notorious for nitpicking) there reviewed it and gave me a book-long list of nit-picks including jewels like "why are you using a switch statement instead of if else? Some devs may not be as familiar with swtich statements, so its probably better to stick with if else", and I had teams from other departments on my ass "where is the feature? We want it!", so I made the call and told the most senior dev "Since the comments are not concerns about something not working but just quality of life suggestions, and given that XYZ are waiting for this to go out, and given that two other people already approved it, I think we can just proceed for now, but I will make a new branch to address these concerns still... just not a reason to hold off on releasing", and then I got fired like two weeks after that after having had an endless string of 1 on 1 interviews saying I was doing great.

[collinoly]

When I was a new dev I had a code reviewer who would nitpick but would still approve my code. I appreciated the additional feedback but only because it didn’t block me. If I had time to make the fixes I would. But if not I would try to transfer their advice to the next PR