STOP Nit Picking In Code Reviews

[@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

[@pianissimo7121]

My code review is either, "don't bother me, i just approved it and couldn't care less about this" or it's "can you change new line bracket instead of inline bracket"

[@YumekuiNeru]

@@pianissimo7121 inline bracket is more than just nitpick to be fair

[@SuprBrian64]

We have nit comments almost every code review on my team but we still approve the PR.

[@7h3mon]

@@SuprBrian64I love this idea!

[@HansyProductions]

@@SuprBrian64 I do this too. If I just have nits then I approve and consider them more as optional suggestions

[@viniciusleitepereira5571]

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

[@paulalves966]

I like his videos, but he is a bigger loser who uses a language that was heavily influenced by C# and F# syntax and he is not even aware of that. 😂

[@vikingthedude]

U also like being handcuffed huh

[@paypalmymoneydfs]

@@paulalves966 🥵🥵🥵

[@JeremyAndersonBoise]

@@paulalves966 😱 that escalated quickly

[@attckDog]

Hey ! I'm a C# loser !

[@taylorallred6208]

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.

[@xAtNight]

That's how I do it aswell. I'll be like "nitpick: this name could be better: newName", approve if the code is fine and just call it a day. If they agree with the nit they can fix it.

[@alexwithx2539]

@@xAtNight this is the way IMO. You can still leave nitpicks but just don't block the people from merging the code if it looks fine otherwise.

[@1zui]

That's how I understood it from the beginning and I find nitpicks on my code quite helpful to improve. But of course, the amount of nits should be reasonable.

[@PeterAuto1]

I normally add a notice that it can be ignored in my nitpicks

[@weirdworld3734]

I also do the same. I will add that "This could be changed a bit but not needed as of now. Just remember for future". And then I approve the PR

[@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!"

[@Turalcar]

Same. Except they don't even pretend to look. It's usually "Sounds fine, push to staging and we'll see"

[@lucass8119]

I have to hunt people down and threaten them with violence to approve my PRs. And half the time it's a single line change checking a var isn't undefined before using it (who tf wrote this code?)

[@jakestewart7079]

Yes the good ol' rubber stamp. I've been in both environments and there are pros and cons to both but honestly the spot check/rubber stamp approach isn't as reckless as it seems if you have good testing/qa in place. Also I think you can usually trust longer tenured devs. Usually when someone is new, that's the time to be extra critical of their code so they start to adapt to the team's standards... Or if they're good, the rest of the team can learn something. Anymore, I just take a quick look and unless something is completely backwards, terrible or checks are failing I'll approve. Odds are that tests or QA will uncover anything wrong with the functionality before it makes it's way into prod... Depending on your process. Really it all depends on your organization and it's processes. I would bring it up in a one on one with your boss or lead that you'd like to get actual feedback in PR's.

[@Skovgaard1975]

Yeah I don't want to read your lame code, sorry.

[@charleslambert3368]

ship it and we'll see

[@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.

[@sircalvin]

i dont know whether to thank you or not, because this is eye opening but also its a curse of knowledge; now i really want that feature and am upset that (as far as i know) no framework has it

[@oOShaoOo]

mutation testing frameworks do this kind of flip and more.

[@michaelzomsuv3631]

I don't even understand how this can happen because when I write a test, I write very specific asserts for what needs to happen. If someone writes tests that can pass no matter what, I think the problem here is not the tests.

[@cornoc]

you have an example of a test that would still pass with asserts logically inverted?

[@unaiarrieta158]

@@cornoctests that do not have any asserts. I've seen some of those.

[@henriquesouza5109]

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

[@Jabberwockybird]

I get the feeling that Primegen would disagree with you on consistency.

[@Microtardz]

Depends. Are we doing business logic, or are we in a library. If we're in a library, consistency is paramount. The people interacting with the library need to know it is always going to handle things in one way. If we're just doing business logic in a project, I don't care about consistency anymore. And the reason why I don't is because no matter who touches that code, if it's not the person who wrote it, they're going to have to read the entire thing anyway to understand it. You cannot expect consistency with business logic. People think about and approach problems in entirely different ways. And as long as it's not fundamentally against the architecture of the code base, it doesn't make much sense to try and force a way of doing things or a way of styling things. It is a good idea to have a style guide standard. Hell, it's an even better idea to setup linters/code formatters that enforce that style standard. But if you're having to enforce the standard ad-hoc in a code review? Just give it up it's not worth it. If you really care, merge it, run a code formatter on it to fix it, and resubmit it with the updated style. I will say, none of what I just said applies to systems level programming. If you're working at the systems level there should be a hard standard that is followed religiously by the team. And that standard should go above and beyond the average standard even into specifying architectural demands such as whether exceptions can or cannot be used.

[@ttrev007]

i think the point was to automate that part

[@sirhenrystalwart8303]

Consistency is not one of the pillars of programming. Have you looked at the linux kernel? It's full of inconsistencies, yet the world runs on it. Rather, the importance of consistency is a lie mediocre, OCD, programmers tell themselves so they can bikeshed about irrelevant details instead of doing real work. It's unbelievable to me how many programmers just mentally segfault when confronted with these small varying details. IMO, this is indicative of a very lacking, amateurish skillset. Yes, I believe if you get hung up on these stylistic nits that you are not a very good programmer.

[@andercorujao]

the whole article is about this, you stop nitpicking, the codebase becomes inconsistent, people gets less annoyed at reviews, they also focus more on the important parts what is better, consistency or the other things ? they have their opinion, every choice has its cons and pros

[@05xpeter]

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

[@bernardcrnkovic3769]

agreed, me too. one more reason i hate working remote

[@05xpeter]

​@@bernardcrnkovic3769 I was working in a remote team where we had the policy to do most peer reviews in calls. It worked super fine.

[@flayms103]

Used to have them and they were goddamn awful. Boss had like 50 changed files open and was nitpicking every single thing for like 4 hours, just because some stuff wasn't prematurely optimized even though it had 0 impact on the actual runtime of the code. Always went out of those meetings feeling like shit. Now i am getting the nitpicks as comments on the repo but there i can just ignore em

[@bernardcrnkovic3769]

@@flayms103 well, it just proves that you can very well be shitty colleague even IRL. however, i believe it at least has human factor so the nitpicker (if thats a word) can cut back on being annoying.

[@kevinwood5048]

It definitely depends on the person doing the review. But if done well it can definitely save a lot of back and forth and avoid other pitfalls that can happen with remote/asynchronous reviews.

[@stefanms8803]

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.

[@sdfsdf421df]

consistency. If you have consistent code base, it will be more clear to every one. Do not nitpick is just not aiming for perfection. If you stop nitpic, others will follow and codebase will quickly turn sour.

[@wegvS]

Exactly. Like I don’t care if we use getValue() or value(), 2 spaces or 4 spaces or what ever but please stick to one or the other in a codebase

[@briumphbimbles]

Yeah providing the person with OCD isnt wrong.

[@tsh4k]

Then the person with OCD can refactor the code later. Integrating code should take priority over blocking over small issues.

[@sdfsdf421df]

@@tsh4k surely not. This is what low-style-often-extra-lazy coders want: to somebody else to finish their undone job. ~ if you have project managed correctly, you have coding standards, implementation patterns etc. Critical projects has them (like nasa for example, funny ones btw.) Everyone know them and adheres to them. OCD freak can go elsewhere if he wants something extra, just as slacker with 'code with priority' refusing to deliver standard quality. ~~ There should be exceptionally low number of 'this has priority and has be committed now' events. Like 1/year. Otherwise your project mgmt sucks. If you have problems with nitpicks and have no standards, once again, your project mgmt sucks.

[@patrickkhwela8824]

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.

[@SuprBrian64]

One of our metrics that are tracked for "performance" are comments on CRs. So we are encouraged to write nit comments but we approve the PR if it's without major issues.

[@jmickeyd53]

@@SuprBrian64 that might be the single worst case of Goodhart's law I've ever heard.

[@freedom13245]

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

[@sullivan3503]

Actually, you're getting paid for it. You're getting paid to learn. Knowledge work is a privilege.

[@streettrialsandstuff]

Exactly, why TF would anyone hate on nitpicking. Especially considering that they are non-blocking and usually mean "next time this way would be better"

[@Skovgaard1975]

But sometimes it is just a matter of style and then it gets annoying really quick.

[@evancombs5159]

@@Skovgaard1975 some styles are objectively superior to other styles.

[@samuelwaller4924]

​@@evancombs5159nit: Be sure to capitalize the first words of sentences.

[@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.

[@neociber24]

What are we actually calling a nitpick? if it improve the code isn't nit picking. For example an incorrect usage of const, let, var in JS. But prefixing a private variable m_variable, I think is.

[@robonator2945]

it's a lot like movies, most things wrong with any given movie are going to be nitpicks, but how many nitpicks are you willing to accept before you realize there isn't any non-nits to be picked. Ant Man Quantumania's issues are largely nit picks, (there are a few big issues to be clear, I'm not saying EVERY issue is a nitpick) Cassie just ominously saying "drink the ooze" instead of "this is a magic translation liquid, drink it", every species sounding the same before drinking the ooze despite supposedly all speaking in entirely different languages, the fact that ants are a biological monarchy and literally can't socalist or technocratic, MODOK's face, "just don't be a dick", etc. A few bad lines or CGI is an otherwise excellent movie would be a nitpick, but when the entire thing is just slightly broken all the way down, there isn't anything there that isn't broken.

[@DF-ss5ep]

Oh, I accept it without problems, but I'll still insult you under my breath, and I've left a company because of that, and only ever raised the issue in my exit interview

[@lukkkasz323]

@@neociber24 Nit picks are minor problems by definition.

[@neonicacid]

While not a full developer at the moment, I am usually working in C# and PowerShell at my job. We had a new person come on our team who took over a critical LOB application. They configured it once by hand then, with no PowerShell experience, decided to let ChatGPT write a PowerShell automation for them. They had me do a code review after a few weeks and functions had like a dozen responsibilities, function names were just applicationName1 instead of describing anything of what they were doing, there were 125 VSCode problems for whitespace or Write-Host instead of Write-Output, etc. It was pretty sloppy. When I brought these points up to them as suggestions on how to improve their scripting, they were almost revolted by the ideas I was giving and acted like it was a personal attack. I get it, we all start somewhere and I've definitely wrote quick and dirty code to get things done in a pinch. But this was something that had weeks of thought and effort into it, and there was almost no forethought of doing things properly/better or asking someone with more experience from the start. They have worked on other things since, followed the same bad practices, and now they're not asking for any advice because it's critical. I don't see how you're supposed to get better if you're not willing to have an open mind to what someone with more experience has to say.

[@asdfasdf9477]

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.

[@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.

[@ThePrimeTimeagen]

again, those can get lost, they can clutter the ackshual important work, and generally giving everyone the right to ignore them makes them meaningless.

[@raph151515]

if it's subjective, keep it out, it it's objective but optional, then it adds value as a conversation about useful stuff, I personally encourage those ones, but they should be marked. In fact mostly the mandatory change requests should be marked to be highlighted, maybe we should have conversation page about modules / features that are detached to the code review which get ignored hidden after merge, conversations should be able to mature and last and can produce tickets and rules.

[@Ruhrpottpatriot]

@@ThePrimeTimeagen Disagree somewhat. By stating the nits you give others another perspective. I have many co workers who are doing their work since the 80s and sometimes it shows. By pointing out "Hey think about this for a second", you tell them, that there's another way. This doesn't mean that PRs should have dozens of nitpicks and five is the absolute most I'd tolerate.

[@kooraiber]

>Our nits are mostly just how to simplify something (i.e. make it more readable) this is a subjective opinion and has no place in a code review.

[@ChillAutos]

@@kooraiber All code is a subjective opinion for the most part. There is 1000 different ways to do just about everything, its all subjective. Nits have their place, in the right environment it stimulates conversation and helps people learn. Ive given nits before and realised I was wrong after their explanation, and vise versa. If a function is 40 lines long and could be turned into 15 lines and be more readable if you dont pull those up in 2 years you have a code base with 60% more code than needs to exist. If the only thing you are correctly is straight up bugs in your PRs then I dont know what to say.

[@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.

[@PieterJacob]

I can relate. Pointing out to what might seem insignificant, can sometimes be indicative of larger problems. Addressing these small issues can prevent potential bugs or performance issues down the line. That's not nit picking... That's saving the company!

[@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

[@alexlowe2054]

This is the way. If there's some coding style that can't be covered by CI tools, and you absolutely feel the need to pick the nit, then at least mark the issue as "not blocking". That way the person can push their code through if they strongly disagree, or if they have a crushing deadline they need to hit.

[@vorrnth8734]

Hm, that way inconsistencies will accumulate.

[@adamfarmer7665]

Yeah, new devs send lots of buggy or less performant code, and sometimes seniors approve it to not make it a headache of training the junior, especially when some of the juniors are full of themselves. Then you get a code debt of old and slow code that most of the time works, but with illogical algorithms and unhandled edge cases.@@vorrnth8734

[@0oShwavyo0]

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.

[@rand0mtv660]

To be honest I think your attitude is really healthy. When I was a junior developer, I wanted to get all possible feedback that I could get as well. I think nit picks are mostly ok if they are explained by the person suggesting them. Sometimes the reasoning might be as simple as "do it like xyz to make it consistent with the rest of the codebase", while other times it can be something way more specific. Of course not all nitpicks make sense and some are extremely biased, but at least if you get a good explanation for it you can see the reasoning behind it.

[@scythazz]

The problem is the nits are not like fundamental errors in ur code but literally things like “can you rename this variable” or “can you squash all ur commits into one”. They are usually things that really don’t matter. And then the change gets delays by days over somewhat meaningless changes. It becomes like they are giving you comments that sound like “you are not coding it like how I would have done it” instead of the code logic being actually wrong.

[@0oShwavyo0]

@@scythazz “can you squash your commits” is not a pointless comment, how else is a junior going to learn the best way to present their changes for review? I’d rather review several concise commits with good commit messages than 20 commits with “FINALLY got that test to pass”.

[@JoaoBatista-yq4ml]

There are a lot of ways to learn that don't involve people actively telling you what to do: you can do code reviews and ask why people do things in a certain way, you can get curious and search how people solve X problem, etc. The problem with nitpicks is that a lot of the times they are preference based and can be a time waster in code reviews, because you are not really improving your code but rather doing things in a way that the other person likes just for the sake of it. You might even get in a situation where another person will tell you to do the opposite of what other person told you to do in a different code review

[@twothreeoneoneseventwoonefour5]

@@0oShwavyo0 you guys review commits separately? I thought people only review the entire pull/merge request and not the individual commits one by one.

[@kipcrossing]

Merge now, fix when a bug is found in prod

[@leakyabstraction]

12:00 Bit disappointed if Prime doesn't think bad structure/architecture is gonna screw you, because based on my experience that's absolutely the most damaging thing that can happen, and it can literally kill projects. I think one of the hallmarks of an experienced developer is that they mainly focus on the structural aspects of code during reviews, and extrapolate in time to see what will most likely become a problematic pattern in the code base, what will increase friction during work, what will cause unnecessarily high mental load due to bad design, what will cause bugs due to e.g. implicit couplings, and what will ultimately derail the architecture of the application and hurt the maintainability. Nit picking is putting focus on things that don't have such a risk or impact associated.

[@JamesSmith-cm7sg]

100%

[@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.

[@ThePrimeTimeagen]

i think we all agree that a good name is required elements vs elementList isn't a reason to get all upset, that is literal preference by someone foo vs timestamps is a clear example of "hey, foo? could we get a more descriptive name here?"

[@programvydas5379]

@@ThePrimeTimeagen I remember a case where a class was named ArticlesScreen. It had a a list of articles and clicking on them would open ArticleScreen. Clearly it had to be renamed and we never made that mistake by sticking most of the time to the prefix -List and not -s.

[@bobbycrosby9765]

​@@programvydas5379 who cares. There's half a million nitpick things that possibly could cause or prevent bugs, expecting everyone to memorize your specific list is a foolish endeavor.

[@henriquesouza5109]

@@ThePrimeTimeagen elementList is just the worse. It's not about right or wrong, it's about consistency. If you name your array everytime differently according on how you feel, then you gonna be inconsistent, which is worse when searching for things (ctrl + f) for example and bad cuz its own definition.

[@matthias916]

the reason some c# devs, including myself, like to prefix private fields with _ is not to show other pieces of code they shouldnt access those fields (because they cant), but rather to indicate to the reader of the code that a certain variable is a field of the class theyre currently reading, its really nice to be able to look at a variable name and immediately see whether its a local variable, parameter, field, etc.

[@ThePrimeTimeagen]

this is the same argument to me as imbuing your variables with the type definition, you are just doing scope definition, but you can always rely on your lsp to tell you things you need to know if you need when you need to know it

[@matthias916]

@@ThePrimeTimeagen I feel like reading code is like listening to someone talk, if they make it clear what they mean you wont have to ask for clarification, which is a whole lot faster than having to interrupt them every now and then. I feel like the same idea applies to reading code, if you can tell what the type of a variable is and where it's defined just by reading the code you won't have to stop and figure it out before continuing. I think it's fine to iterate over a list and call the iteration variable "element" when you don't access any of its fields or call any methods on it, but when you actually want to do something with a variable I think it's important to give it a proper name so you know what exactly it is and what those methods or fields really mean

[@Skovgaard1975]

@@matthias916 Spot on!

[@evancombs5159]

As a C# developer who absolutely hates this practice. If you are writing concise methods, this should never be an issue as you'll never need to do any significant scrolling to see where the variable was declared.

[@radfordmcawesome7947]

​@@evancombs5159 you don't even need to scroll, your editor/ide can tell you right there where the symbol is being used. this is a practice for experienced noobs and furthermore is prone to human error

[@larryd9577]

Single-letter variables for scopes which span a single line are totally fine.

[@EbonySeraphim]

After the self admission from the article writer, I had to double check if it was written by a principle engineer on a team I used to be on. It was the most obnoxious thing and seriously drained my soul trying to push things through dealing with nitpicks for days and weeks, and then afterwards there was usually a new major issue to address. At some point this was sabotage. Honestly, my opinion on code reviews is that style or approach choices should always go in favor of the person who wrote the code if they feel confident enough to vocalize a disagreement to the feedback given. They’re beholden to a deadline not some other, likely more senior, engineer who just feels like imparting knowledge. If you can’t demonstrate a sure or super likely problem if the code is released, then can it. Essentially: LOGAF. Also, all code reviews should address major architectural issues first before getting into bugs and nitpicks that will probably be rewritten anyways.

[@DNA912]

If there are major things to point at in the code, I don't nit pick. But if I can't find any (real) problem, I will probably nit pick. about the example with time or getTime. I personally don't care what you do, as long as you do the same everywhere in the class/module/whatever. Just being consistent is very good if you ask me, and most of the time the LSP recommends a convention for the given language, so just use that one.

[@xBLiNDBoi]

This happened during my first job as a QA. During a triage meeting going over bug reports to determine what needs to be fixed before the release goes out. A developer decided it was time to nitpick the wording of one of my repo steps in front of 10 to 15 people. The last step of the flow I used the wording "verify the crash occurs". It basically was do this action in the app and it would crash, a very obvious bug that shouldn't go out into production. He didn't like the word "verify" because it meant that the bug was supposed to be there. I changed "verify" to "observe" literally in that meeting. It was stupid and pointless, hell the guy could of just sent me a DM in slack or talked to me in person about it. I sort of wished I pulled out a thesaurus just to troll him... all of the things I would of done differently now compared to then.

[@TheTigero]

I’m 100% on their side on this one. Their reasoning was correct, and it was an easy quick fix. Why are you so upset about it?

[@ilearncode7365]

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.

[@gristlelollygag]

that's fucking terrible

[@Sonsequence]

I'm not saying you were wrong but the way to handle that is to get the senior dev in the room or on a video call, have a patient approach, point out some of their nitpicks you agree with and ask if it would be ok to do them in a follow up PR given the practicalities of the situation. Unless he thought you had written a lot of truly inadmissible code then it probably would have been a yes. On the other hand, I have often found myself reviewing really terrible, chaotic, repetitive code that was clearly hammered not designed. I'll have caught some bugs in it, get a revision which deals with those but it's still a mess and I know other bugs are lurking. This guy will say "but now you're just obstructing for the sake of style preferences". Each time I give in we soon find out what bugs were in there and I end up rewriting it. Bad programmers often refuse to accept that a negative review is not a blockage on your finished work, it just means the task is more work than you thought.

[@gregorymoore2877]

Wait... he actually put "Some devs may not be as familiar with swtich statements" as a comment in his review? My answer to that would be "that's ok, if they come across the switch statement I wrote they can familiarize themselves with it and they will no longer be 'not as familiar with switch statements'"

[@vikramkrishnan6414]

99% of nitpicks belong to the realm of linters and formatters. Edit: Commented before watching the full video.

[@Christobanistan]

STOP VIOLATING THE COMMON LANGUAGE SPECIFICATION GUIDELINES!!! It is YOUR responsibilit, Prime, to follow the coding style of the team. PERIOD! I have a feeling Prime was a terrible C# developer. Conventions are good. Coding style guidelines are good.

[@moodynoob]

The nit that has stayed with me to this day is when an engineer commented on my every use of a named function declaration and said to use fat arrow instead because it's "ES6 style".

[@jannemyllyla1223]

Agree, in my last job as QA Lead I put a no nitpicking rule in place. If some team feels strongly about some nit there needs to be an automatic check for it configured and taken into use.

[@Turalcar]

Why is it QA lead who made the call?

[@jannemyllyla1223]

@@Turalcar I gathered metrics and nitpicking came up as a clear waste of time. It is part of the responsibility to keep process efficient. The time wasted with pr nitpicking makes getting features out much slower, sometimes adding days.

[@danielvaughn4551]

I hate prefix-based development. Underscore, dollar sign, @ sign, I hate it all.

[@BenRangel]

Addendum: for brand NEW teams nitpicking can be allowed for a few months. It can often be a way to start more important discussions about general code structure. If you entirely disallow nitpicks early I think some devs will feel like they aren't allowed to voice their opinions. For example if they want early returns and avoiding deeply nested ifs - that opinion might initially voice itself as a nitpick in a single PR but then turn out to become a valuable general agreement over code styling

[@SimonBuchanNz]

Literally my definition of a nit is "here's something you might not have thought of or know about, but it's not a problem for the PR", and it's always called out as being such. I make sure they know that i only expect them to make these changes if they both agree and are already going back in to make other changes anyway.

[@lloydbond13]

I love it when I add new features to old code that nobody has touched in years. The new linting and formatting policies from the pre-commit take affect and now the entire file is part of the code change. There's always some Super Brain that wants to use your PR to address code quality issues from three years ago.

[@saryakan]

Function names are important. Bad names can obscure what the function actually does. Variable names are less important. Kind of depends on how long lived and wide reaching the variable is. If it's a global variable, which is accessed all over the code base, having a good name is actually somewhat important. For params in an anonymous. single-line function nobody should care.

[@rand0mtv660]

Yeah I'd agree with you on single line function variable names, those can get shortened to x/y/z as far as I'm concerned. Everything else I prefer actual proper variables. I also dislike when people use "e" for JavaScript event listeners event object instead of writing something meaningful like the word "event". I never understood what's the benefit of shortening that unnecessarily. Typing 4 less characters won't really save you that much unless you write code in Notepad (not ++) or something lol

[@saryakan]

That said, anyone writing elementList instead of elements should be chemically neutered and forever to work in the mines.

[@lucass8119]

This is an extremely good point. The lifetime of the variable is very important! for(auto e: some_container) does NOT require a name bigger than "e". One character is enough for these quick temporaries that get destroyed in a couple lines. I mean, ideally, don't even name temporaries if you can.

[@gabrielnuetzi]

One often forgets one massive point here: We read code more often than we write it (10-100x) and what one puts into the code base matters! What is a nitpick an what not is pretty much a fine edge and its just not black an white: pointing out stupid types in variable names like “elementsHashMapWithIds” can be seen as nitpicking but its actually not. Naming is hard, and often finding a better variable name directly makes the code so much better to understand, its about sorting out the way too unspecific variable names from the ones where a proper name actually matters and just “elems” is misguiding and not helpful, thats why senior deva exists because you need experience and a good amount of abstraction capabilities. Thats why you should have best practices or tools to detect them, such that the reviewer does not blame but the tool blames you. Simplifying code is not nitpicking when you take the above fact seriously, if you have a clusterfuck of for loops with a cyclomatic complexity which shoots to the moon in 15 lines, and you point out that this is complected and can be made more reader friendly thats actual very good, treat your code as you would write a book, nobody likes to read pages of vomited brain dumps and then also build up an understanding for it. Logic/safety first, reader second. Formatting nitpicks should not ever be discussed, there are tools.

[@gabrielnuetzi]

But I agree: Nits should only be things which a tool should already have pointed you out to in CI. full stop. Never discuss them, includin whitespace changes, include order, etc etc, thats the tools job and does not belong into code review

[@Christobanistan]

@@gabrielnuetziI agree. Prime should follow the team's coding guidelines and stop bitching about fitting into the team. But that stuff is best handled in CI, not in code review. Kick it back in his face every time he checks in. Bad naming is a crime, though, and can't be caught but in person.

[@NotMarkKnopfler]

Wanting code to be perfect is a tricky one - because "perfect" is of course entirely subjective. My idea of perfect is not yours. And you're not wrong. And neither am I. We have just had different experience paths. For example, I often like to invert if conditions to avoid nesting. A colleague of mine doesn't. I think I'm right. He thinks he's right. Both approaches produce perfectly functional code, and the compiler simply doesn't care - it optimises it anyway. I just believe my code is easier to follow (doesn't nest as deep). He believes my approach is counter-intuitive as you're often testing for 'opposite' of what you would normally be testing for.

[@streettrialsandstuff]

Not necessarily. A nitpicking might be because you've missed to adhere to established conventions, for instance, like the mentioned underscore example. Also, about subjective parts, that's why those are nitpicks in the first place, right? So that someone might tell you what might be better and it's up to you to decide if you agree or not because the nitpicks should be non-blocking.

[@TheTigero]

Except in this case you are correct and they aren’t. Early return is paramount, and deeply nested code sucks.

[@bgdgdgdf4488]

​@@TheTigerothere is also an argument to be made that a single return makes debugging quite a bit easier. Op specifically said it doesn't nest deep.

[@Christobanistan]

I wouldn't bring that stuff in code review, but I'd put a Linter on it so it kicks back during integration. But to say things like that are just "nits" it b.s. When writing C#, you must follow the CLS. These are Microsoft issued guidelines that keep code highly readable and consistent so when the next dev comes in, it'll all be easy to understand. And yes, capitalization matters because it makes it easy to tell what's a private vs public variable or a method, a const, whatever, at a glance. Sticking to VerbNoun is good because time() could be doing anything with the time, and descriptive names ARE IMPORTANT!

[@atikenny]

One man's nit pick is another man's "How dare you?"

[@MungeParty]

I actually really like underscores for protected and private members, It used to be a C# convention so it's not about whether the language supports access modifiers.

[@TerryQuiet]

as a junior dev, Me liky when people nit picking my shitty code.

[@raph151515]

true, good point, at the beginning you need to spend time thinking about small details, but a senior will be bothered quick because he knows how much he can deliver if no obstacles lie in the way.

[@andreysudakov8377]

As a C# loser I'm really rooting for these guys to finally invent static code analysis. They are really close

[@n00dle_king]

My preferred approach is to include all nitpicks in a first pass but use language to ensure they know its an opinion. And if they push back I'll "Nevermind" any issue that could be a preference. But, I think the act of communicating preferences between team members helps push towards a unified code base even if it doesn't immediately lead to code changes in a PR.

[@raph151515]

I don't like the game of trying to refuse a change request then nervously waiting for the answer. A PR shouldn't be a personal pressure game, don't forget that devs are closer to autistic spectrum than any other jobs, the rules should be simple to apply (clear and easy to check) and they should never be personal. Personally you can still call the guy and have a conversation about why he prefers this way and why you think your way is good, but don't make it a required step to merge.

[@flarebear5346]

​@@raph151515I would like to nitpick your comment but I have autism

[@markparker6499]

“One’s ‘scrupulous critique’ is another’s ‘principles to cherish’, Let not the assiduous be misconstrued as the fierce.” - Me

[@mrmesketo]

One of my PRs was blocked because another dev wanted me to change some divs to spans because it was "more semantic". Also those divs were pre-existing and not part of my changes.

[@Jabberwockybird]

🤦‍♂️and changing block level elements to inline is more than just a semantic change

[@thomac]

I worked a lot with legacy codebases, and there's always that one guy that now and then wakes up and starts asking for more refactoring in nonsensical scenarios, like small bugfixes, or in large features that barely touch old code. Dude, if you want something, create a ticket, or talk to a product manager, don't block other people's work.

[@mbfun9298]

I got a good recommend on this from coworker back in the day, which is to actually prefix my nitpicks with "nit:" and to also outright tell a coworker when I review their first MR/PR that anything starting with nit is just a nitpick that you don't need to address in anyway for me to approve your MR/PR, that way I can get full usage of my nitpickiness and cause as little damage to the coworker as I can ^_^

[@isodoubIet]

Adding underscores to variable names is useful if you want to provide a getter for that variable and what to keep the name short (e.g. foo.bar() instead of foo.get_bar()). I also find it useful to immediately know if a variable is a member variable or a local by just looking at its name. Conventions when consistently followed can be very useful.

[@regizer2399]

I am a nitpicker and I'm trying to do it less, but when it's a team consiting mostly of juniors, then they usually don't choose to do it one way, but rather they don't know they could do it another way, so it is useful for them to know. Also, if I'm the one who deals with 80% of bugtickets, because the others either have no idea what to do or I have to guide them through it and I have to debug the shit code, then at least make it closer to my kind of shit (which is not underscore or not, but definitely includes proper variable names, which everyone admits are better in the end) so I don't have to scroll up and down to find out what "res_a" and "res_b" is amongst the 3 letter variables. To be fair most of my real nitpicking is done with suggestions which can be applied in the PR directly, and would be mostly avoidable if a formatter was used (which people refuse and forget to do, because if you format then I accept even if I don't like it) My only real crime is with the random extra spaces that do nothing. I'm sorry I know I'll burn in hell but I must suggest to remove. Also, msot people have different ideas about nitpicking. Some old colleagues thought everything is nitpicking when they're in a hurry, which is kinda fair, I usually am not that picky when it has to be done fast, but if it's 5k C code with references and pointers mixed for some microcontroller that can only be run in some shit IDE which can't lint properly, I will ask to have some consistent naming. Why? Because after refusing this, the unreadable shit was tried to be debugged for months by multiple people to find multiple memory leaks which would actually have been quite visible with proper naming.

[@jmickeyd53]

Apparently I have a completely different definition of nitpick than everyone else. I use nit: for cases when the code is technically functional but actually sub optimal in some meaningful way, bad abstraction, performance, etc.

[@ThePrimeTimeagen]

that isn't a nit, that can be a suggestion

[@ChillAutos]

@@ThePrimeTimeagen What is a nit then? To me what @jmickeyd53 said is a nit and how ive always understood it. Example might be using a useMemo when there is no need. Creating more state than needed in react instead of deriving that value from existing state you already have. Complexity where there doesnt need to be any. Endpoints inconstantly named etc... A nit to me is just "Hey your code works but it could be better, here is how, feel free to ignore this" (I dont actually write it like that but thats just the intent).

[@Leipage]

@@ChillAutosA nit is generally a stylistic preference that doesn’t change how the code actually runs, and is usually only a debatable subjective preference. A few examples: 1) requiring a variable name change from elementList to elements. 2) requiring a change from a for loop to a foreach loop, 3) requiring a reordering of includes, 4) requiring a change from an if/else to ternary operator, 5) requiring they remove an else statement and use early return, etc. And yes, I’ve personally had to deal with each of these nitpicks (and many more). They’re very annoying, to say the least, and just a pointless distraction from the actual code that matters.

[@jmickeyd53]

@@Leipage IMHO this should never even get to a review though. Have a style guide and a linter. Code review is not the place to have these style discussions.

[@Leipage]

@@jmickeyd53 Well that's the point I'm trying to make: there are many nitpicks that are not included in the style guide and cannot be fixed with a linter. None of the examples I gave above would be in a style guide or fixed by a linter. From what I've seen, nitpicks are usually personal, subjective opinions where the reviewer is trying to force their subjective opinion on to someone else, but it doesn't actually improve the code. It's ego based. It's like someone saying "I see you are eating mint chocolate ice cream but I like strawberry ice cream better so you should change to strawberry." I prefer to use Google's policy: "if a specific stylistic choice is not mentioned in the style guide, then defer to the author's choice."

[@spectr__]

I have a colleague that is super into "Clean Code", I avoid tagging him into reviews...

[@salamonrosenberg-vh5xo]

....hey Bob, I noticed in your last PR that your structs have comments above the parameter vs inline.... Did you get the memo ???

[@raph151515]

well done, but it should be rotating

[@spectr__]

@@raph151515 rotation is kinda loose, so I wait for the simpler PRs to tag the nitpickers

[@gregorymoore2877]

@@salamonrosenberg-vh5xo Yeah, I know, I just forgot that one time. But I've already fixed it so it's not really a problem. 😉

[@LoZander]

Now, I'm about to nitpick, but I'm also going to justify my thoughts. On the time() vs getTime() thing, I feel you could construe time() as the verb, and thus think time() is going to time something and not just return the current time. Generally, I don't think getX adds much if anything, though. Maybe you could make the case that it conveys no side effects or something? I don't know. You could also argue that when you have a setX, naming getter getX makes for nice symmetry. Ultimately, I don't think it really matters much, if at all.

[@corruptedsave145]

getX is nice, but sometimes I am not sure what exactly I should be doing, type get and my IDE shows me a list of 150 things. More specific naming like timeX() helps when you're tired/reviewing something older. But yeah, doesn't matter much. I just don't like get that much because I get assaulted by brain fog at the worst times.

[@lukkkasz323]

@@corruptedsave145 It shows you a list of all things you can get, isn't that what you wanted to do? Seems perfect to me.

[@michaelzomsuv3631]

get/set is not a problem. It's a symptom.

[@FirroLP]

Should just be x.time for getting it tbh, like any other object access

[@LoZander]

@@FirroLP if the language has a mechanism for making getters that behave like fields but still protect from mutability, then I agree that x.time is best. But my point is that if the getter takes the form of a normal method x.time() then this could maybe be construed as the verb and imply that the methods times some kind of event or something. Because time is not just a noun but also a verb, in this specific case, you could maybe argue that getTime() actually helps make it less ambiguous. But generally, I do think most people would assume that x.time() is just a getter.

[@krige]

9:08 nitpick: you mean you have an X coming

[@pot42]

I love the fact that I passively listened to this video, and yet my body language was just the same as if I had been reading clean code: slight smile, head nodding, and a violent urge to create an elbow/jaw interface for the nitpicker.

[@harry_jms]

My relationship with PR's drastically improved when I stopped seeing comments as critiques, and more as a conversation. But nit picks do annoy me so much 😂

[@ashutoshkaushik9118]

Just wrote a whole package with my private variables with an underscore at the start... Gonna make a refactor PR right away 💀

[@gregorymoore2877]

Why?

[@ashutoshkaushik9118]

@@gregorymoore2877 i didn't actually do it dw

[@patrick.miharisoa]

Trailing underscore or ''m_'' prefix are especially useful in a language where one can access method and attributes without referencing to the current instance (this or self)

[@Uuppi]

I would love to see an online survey on how people do code review comments. There could be a list of code review scenarios, each with multiple choices to choose from. The questions could include topics such as how nitpicky you like to be, what's your tone of voice in your comments ("you should do X" / "Could X work better here?"), How small or large should pull requests be, etc.

[@thomac]

tone is definitely important, but even more important is context. you should write why you ask for a change, rather than wait for the other person to inevitably reply with questions.

[@batlin]

Probably my biggest pet peeve can't even be called "nitpicking", but just holding up PRs because of totally arbitrary personal preferences. At least (some) nitpicking can lead to the dev going "oh... yeah I see what you mean, that is slightly objectively better", but I'm talking about "hmm, instead of 'CameraPosition' can we use 'CameraLocation' because... I personally like that word" and "can we rename this package from 'tools' to 'utils' for absolutely no particular reason even though it's going to cause 9 million merge conflicts that 10 people will have to fix? No worries, if not I'll just go completely silent and passive-aggressively stall your PR for the next 3 weeks". If it's a nitpicky opinion-based change that's very simple, I might do it to avoid getting stuck in PR review limbo, but otherwise I'll ask if we can just merge the PR now and maybe change packages/naming or other nits later (preferably, done by the person who wants that change made). Sometimes it works.

[@BenRangel]

Trying out a new approach for a month is such a great concept. My team does it all the time when contemplating a new idea. Most of the time the new idea wins by default after the trial period. Quite often people are against changing something - they wanna "keep doing what we're doing".

[@user-ot54ht]

There's probably some oversimplification of the issue or even a missing perspective. In projects that involve big teams of both developers and users, code standards are important - you want the code to be self explanatory on one hand and "uniform" on the other hand. The best way I've seen of enforcing a coding standard is by having an explicit "style guide" document, in some cases referring to existing documents like the ones Google published. Nitpicking beyond the style guide was at least ignored and at most frowned upon, but within the style guide it wasn't even considered nitpicking. In my personal view, code readability is about minimizing the cognitive load of the developers r/w-ing it. For instance, in one of the projects written in python I had a new developer come in and ignore the style guide of providing type hints in functions. His view was "these types are dynamic anyway, and the variable names encapsulate them" - no, type hints are important because some of us don't want to read the source code of your function to understand what's the interface to it. Also intellisense works much better with type hints.

[@radicaledo8737]

in all honesty i do like/prefer the underscore for private methods/variables. my reasoning is basically this - i read code more than i write code. generally i think lots of developers do this since the reading part is necessary to writing part. and by having those underscores i already know that this variable is private or not, don't have to keep that info in my mind and don't have to evoke some TS hints to tell me that this is in fact a private thingy (or not)

[@daddygromlegs1044]

As a Junior dev, I LOVE nitpicking. I don’t know the best conventions or if there is a more idiomatic solution so I always love when seniors roast my code and get as nit-picky as possible. I don’t take it personally at all. Might change when I’m a senior but that’s my take for now

[@jmon24ify]

the nits you are describing and what is being talked about in this video are not the same. The nits he is talking about are co-workers are requesting, sometimes wrong, changes that brings no value to the code you wrote. They are being nitpicking not because they are following best conventions, performance reasons or anything like that. They are doing it just because. I know that it is hard to believe but unfortunately at some companies, that is how it is. Most of the time it is the result of the politics established in the culture to make themselves seem more valuable than what they actually are. That brings a toxicity. You may not catch on at first but you will eventually and it would spark negative feelings towards your coworkers and work situation. Of course because of those developers are the way that they are, when it is time for peer performance reviews, all they can think about is the amount of times they nit you, completely forgetting all the great work you may have done, and give you an awful review which can ultimately cost you your job. I telling you this out of experience since it has happened to me and I have see it happen to others.

[@flashgnash]

A company I worked at once had a system where one of the requirements for getting your bonus at the end of the year was finding over a certain number of issues on average per code review, nits were kind of essential there especially when the code review was for 2 lines of code There was also a requirement to keep your own average number of issues found on code you'd written below a certain number

[@KeyYUV]

I once worked on an older code base where variable and function names were inconsistent (camelCase/snake_case). I fixed a minor bug in a library that changed a few lines. The reviewer nitted the parts I didn't even change and made me change everything to snake case. The worse part is some functions are interfaces that are called by other modules. So my PR went from 2-3 line change and minor version bump to multiple PRs for all affected repos and a major version bump.

[@dripcaraybbx]

We had this rule where markup got 2 spaces and all the big boy toys got 4. Following it to the letter turned into double-tabbing inside script tags

[@xen2297]

Variable names are so important, though. Especially when working with non-native speakers of English. Every bad variable names causes cognitive load which makes it harder to read and understand, and therefore likely to be annoying to maintain in future

[@Jabberwockybird]

I've worked with non english speakers, and a big problem is they don't seem to care to understand. They create the bad names themselves. They don't know the meanings of words, so they just go by context of discussions and everything is tribal knowledge. Names are based on temporary project names that have no meaning to what the code is doing. Example: Project mickey mouse is where we are going to create a feature that allows customers to pick their favorite disney character. So a class gets called MickeyMousePicker. Never mind that mickey is only one of the character choices and the class isn't specific to micky mouse. Because at the time, everyone knows about the project and what it means, so f**k maintainability. Everything will be called MickeyMouseBla. And one can't be too upset about this because they don't speak english that well, so it's just xenophobic. Except it's not! Words have meanings!!!

[@Tom-jy3in]

@@Jabberwockybird As somebody who was born in a non-english speaking country, I can see a direct correlation between people who have bad english language skills and terrible naming sense (if you can even call it that)

[@simonhrabec9973]

I don't see problems with nits. I just see that I chose a suboptimal name or something and just accept the change. I don't understand why I should be offended.

[@Jdinrbfidndifofkdndjoflfndjdk]

OMG dude, I've been here. So much back and forth

[@leakyabstraction]

13:00 What he is describing is the conceptual difference between testing (minute) implementation details (bad), versus testing actually expected application behavior (good)

[@adnan37h]

Sometimes there are devs whose English is just not good enough to come with an adequate variable name. I have to nitpick those if I don’t want some variables to make absolutely no sense

[@AdamLeis]

I'm convicted by this. I definitely nitpick. I've swallowed a lot of complaints or raged to myself. Most times, it goes away. Things that come back 2 or 3 times, I take time to think it through and figure out wtf. Sometimes though… I'm at a keyboard when I should be walking away. Then things get typed…

[@LordOfElm]

Enjoyed the article review. Funny that the first 5 minutes could have been summarized with "naming is hard". I feel similarly on signal to noise with regards to security to accessibility. Balancing tradeoffs is also hard. It's interesting that everyone seems to build their own preferences over time, especially when they often overlap. Is there a clear point where those preferences go from experience based safeguards to baggage?

[@user-qr4jf4tv2x]

a healthy team is a willing to go against what you think and try something else. - ThePrime

[@SteveUrlz]

Yo thanks, I am actually such nitpicking guy. Just invented an improvement to my review process. Added the following: 🟢 Light/trivial improvement suggestions (naming, formatting, logic shortnening) 🟡 Moderate/should change (may lead to issues down the path) 🔴 Important/must change (bugs, incorrect behavior, etc.) Where the reviewed might skip on GREEN comments if they feel like it's a lot of work to refactor or do not fully agree. This will be great!

[@csy897]

Unit testing got me to write better code and refactor better. Also, there were parts of our application that we could never test because no one knew how to and it always left bugs or unnecessary calls/renders so I spent a lot of time learning how to test, creating tools and templates to make it easier to test. But it came to a point where we had too much tests and our cheap pipeline was taking way too long to run. And I wasn't even going for 100% coverage! So, ever since then I've been in a ditch thinking, ok so don't test too much but what is too much and what is too little? I think this is the best rule I've heard so far. If you can't get it right the first time, write a test for it. I think that is fair enough. But also, we should do integration tests, I mean, according to the product owner's story requirements.

[@ericb7937]

This is why you need a PR comment convention. It helps distinguish between blocking and non blocking comments. IMO, nitpicks, questions and comments are still important discussions but being explicit that they are non blocking is vital to make intent of the comment known. Start each comment with either: nit, praise, suggestion, issue, todo, question, thought or chore. And then come up with your own company convention

[@Slashx92]

General design patterns and code styles should be enforced wjen relevant. I agree variable names are 9/10 times just a nit, but for some things we (my place of work) use conventions to know easily what is being done in the function: things like o_myTransaction vs nsr_myTransacion, where the first is a general object transaction, and the second is a netsuite record transaction (the erp we work on)). In this case is knda meh, but if you use tx, I’ll ask you to change it, for example

[@francisgeorge7639]

Naming suggestions can be a useful nit as the reviewer can often have a clearer overall view or spot ambiguity.

[@fullmastrinio]

One of the team leads I used to work with used to rewrite my commit messages when I open a PR. My messages were clear and described what the changes are, nothing inappropriate or wrong, guy just didn’t agree with the wording. The most bizarre experience I ever had

[@yaksher]

I feel like there is room for "minor nitpick suggestions" if you have the time, but they should be clearly marked as minor suggestions and placed separately from the important stuff. Like, you can have a section with a bulleted list of "minor suggestions" at the end of your response and that's relatively harmless, but it needs to be very clear that these are not the important part. Hell, most minor nitpicks are probably no faster to describe than fix: just make a separate commit of your own that fixes them if you care so much. In a similar note, I'm a head TA for a class which is many things, but it is also a class that teaches people to write proofs and teaches them how to use LaTeX and such things. Thus, it is important for the class to comment on small stylistic stuff that I would ignore in a higher level class, including stuff like bad typesetting. But also, it dilutes the feedback. The solution the prof and I settled on is that substantive errors get annotations, which are clearly visible on the page. Minor nits go in the "other feedback" box explicitly labeled as minor comments. This provides a clear separation between the important stuff and the less important stuff and doesn't make it demoralizing to see 700 annotations everywhere on your submission on how everything is wrong.

[@Slashx92]

Another big thing about personal persoective, is whitespace. I’m a very “shapes and colors” kind of person, and I have coworkers that are a lot more “everything compact so I can read it” type of person. The compromise is that some whitespace rules makes sense and can aid maintainability, etc., and should be encouraged. But everything else should be personal opinion (things like for loops with or without whitespace above, separate variables by an empty line to “group” them, etc)

[@RobalexGaming]

About time() and getTime() -> In my experience (which is mostly in the embedded world, so it may be biased) most engineers would see them as two different things. getTime() would simply return a value that has been produced by some formerly called method or asynchronously, while time() would more likely do the thing to produce that value (be it tick/timer subtraction, or anything else).

[@apollolux]

A couple of years ago, I had an interview for Facebook (second time doing technical screening) and the interviewer spent so much time complaining about the names of my variables in my code that combined with the negative interview process I had with them previously (the first time I was at final round but they delayed scheduling it for so long that they hired for the positions before finally interviewing me) it completely turned me off to working at Facebook entirely.

[@PieterJacob]

Haha, great video. Guilty! I'm a great nit picker myself and I recognize a lot what has been said in the article. Thanks for pointing this out. I think this is the reason why my colleagues always look at me the way they do. However, at some degree in my opinion there is not much wrong with nit picking, especially if you are guiding junior developers and make them aware of certain conventions. But at some point you have to let go.

[@fortran31]

"Don't shoot yourself in the foot!" - The Primeagen

[@DarkzarichV2]

As a senior dev I nit pick a lot especially reading juniors code, especially if I'm mentor of the said junior BUT I do give approve if there is only nit picking and nothing serious. There is no reason to delay it if it's urgent or has close deadline

[@superderpyderps]

The only time I'll nit is if something doesn't follow our written conventions (with a link to the rule we agreed on, and the list is very short) or if it breaks away from a common pattern in our code that causes significant divergence. The amount of times I need to add a nit is pretty low because everyone is trying to keep the code consistent anyway. I'll fix nits from others too. If it ever got noisy, I'd want to rethink the approach though, but that might include either better training or rewriting our conventions if they're outdated. I could see a much larger codebase/team making that much more painful though. Working on a team that can check their egos and not take feedback as a personal failing is a huge factor

[@ahettinger525]

Even there, though, wouldn't it be better to make it part of a lint pass? That way the submitter can clean it up themselves before passing it to be reviewed.

[@ArturdeSousaRocha]

In code reviews, I make a distinction between things that need to be fixed (bugs/bad design) and suggestions of minor improvements (actual nitpicks). This can be either verbal or through tools (i.e. comment vs. task). I expect and receive the same. But perhaps my team is better integrated.

[@Jaryt]

My team naturally picked up a process of specifying when something is a nit. We don't reject PRs because of a nit, they're there for you to change it if you want, but they're the least important part of a code review. Especially when the nit is opinionated.

[@martijnp]

The absolute craziest one I've ever gotten was someone reviewing my java code and telling me that an enhanced for loop should be replaced by an integer loop because it was faster. Literally asking me to create nanoseconds of speed increase, at most, while sacrificing code readability. Extra crazy part was that this loop only ran during startup to load some variables. Extra fun fact: this guy also refused to use maps and used switch cases with 100 fields instead. Which, yes, led to 100 case blocks as well. Similar "performance" reasoning.

[@bike4aday]

5:10 This is greatest bit of wisdom that sometimes takes people decades to learn. Every piece of code you look at that's over a week old looks like doggy poo poo.

[@adamcbrewer]

The number of nits are directly proportional to the lack of effort. I'll give WAY more nits if I know every line is another case of they don't give a crap.

[@vert3cx373]

I felt that “C# loser” part. Gonna go check my LOGAF quick.

[@kabal911]

I think I do nit, and maybe to often. If I see something too many times, I might revert to the “seriously dude. Again. I really don’t want to have to raise this same thing again” - possibly not great, but anecdotally has worked…

[@Tuevon14257]

properties: noun names methods: verb + noun Exception is in getter and setter Reason is because properties ARE something, whereas methods DO something.

[@KeeganGaffney]

During my internship this summer, one of my CR’s had 6 revisions, all because of np’s

[@TheSneezz]

I was losing my shit the first 10 minutes of this video, but the end recommending lint really saved it xD You should agree on a code standard with your team. If you dont like underscore for private variables, state your case, take a vote/let the lead make the decision and follow the standard henceforth. What an insane work environment where people can just yolo their own cryptic code styles into a project without care for conventions or code standards. I would lose my shit if i looked through code where some private variables where underscored, some werent, some getters had the get prefix, others didnt, some linq statements were using shorthand, some xyz and others full names. But Lint and autoformating is definetly the tool to use for these issues. I should be able to tell that two classes or files are from the same project/team, it shouldnt just be up to the individual fucking around by themselves xD

[@RockyNeurock]

Great article! I know I once cared *way* too much in the past 😅 Appreciated your bit about testing the wrong things, too. Sandi Metz has some great talks on testing. In one of them, I’m pretty sure she said something like “Most developers write too many tests.”

[@michaelgraflmusic]

All I ever did in my code reviews in one team was to ask the guy to please remove all those left in debugger statements. Often a bunch in one commit.

[@Veptis]

Had a PR being held up by the CI linter being unhappy with mixed tabs/spaces in a string. And the string wasn't even code I wrote. And it has a noqa comment too. so now I always replace tabs with spaces on those files and it passed. No human would have nitpicked that tho.

[@briankarcher8338]

If somebody forces a full variable name in a single-line arrow function, I am going to laugh in their face. Same goes for "for" loops.

[@polltery8747]

I had a really amazing manager, when I opened a PR they would merge in by making the changes themselves to what they felt fit and I would get to learn from it without wasting anybody's time - I was a junior in this case. I honestly think if the reviewer can't fix nitpicks themselves and merge then they waste everybody's time.