STOP Nit Picking In Code Reviews
Вставка
- Опубліковано 29 сер 2023
- Recorded live on twitch, GET IN
/ theprimeagen
Article: blog.danlew.net/2021/02/23/st...
Author: Dan Lew | blog.danlew.net/author/dan-lew/
MY MAIN YT CHANNEL: Has well edited engineering videos
/ theprimeagen
Discord
/ discord
Have something for me to read or react to?: / theprimeagenreact
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))
turso.tech/deeznuts - Наука та технологія
I love when the deadline is today and there is nitpick code reviews that delay the review and merge process by 5 more days
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"
@@pianissimo7121 inline bracket is more than just nitpick to be fair
We have nit comments almost every code review on my team but we still approve the PR.
@@SuprBrian64I love this idea!
@@SuprBrian64 I do this too. If I just have nits then I approve and consider them more as optional suggestions
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.
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.
@@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.
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.
I normally add a notice that it can be ignored in my nitpicks
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
I love to be called a C# loser so early in the morning. Thanks for making my day better, Prime 😊
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. 😂
U also like being handcuffed huh
@@paulalves966 🥵🥵🥵
@@paulalves966 😱 that escalated quickly
Hey ! I'm a C# loser !
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.
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
mutation testing frameworks do this kind of flip and more.
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.
you have an example of a test that would still pass with asserts logically inverted?
@@cornoctests that do not have any asserts. I've seen some of those.
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!"
Same. Except they don't even pretend to look. It's usually "Sounds fine, push to staging and we'll see"
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?)
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.
Yeah I don't want to read your lame code, sorry.
ship it and we'll see
I always find myself regretting letting something apparently unimportant go in review. It's scary how often it's come back to bite me.
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!
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.
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).
I get the feeling that Primegen would disagree with you on consistency.
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.
i think the point was to automate that part
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.
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
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
agreed, me too. one more reason i hate working remote
@@bernardcrnkovic3769 I was working in a remote team where we had the policy to do most peer reviews in calls. It worked super fine.
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
@@flama6608 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.
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.
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.
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.
@@SuprBrian64 that might be the single worst case of Goodhart's law I've ever heard.
I learnt so much from getting my code absolutely bashed by people more experienced than me :)) it’s literally FREE education
Actually, you're getting paid for it. You're getting paid to learn. Knowledge work is a privilege.
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"
But sometimes it is just a matter of style and then it gets annoying really quick.
@@Skovgaard1975 some styles are objectively superior to other styles.
@@evancombs5159nit: Be sure to capitalize the first words of sentences.
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.
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.
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
Then the person with OCD can refactor the code later. Integrating code should take priority over blocking over small issues.
@@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.
@@tsh4k That’s how most juniors think, and it’s precisely why codebases become unmaintainable. Blocking over small issues is a learning opportunity and a good developer won’t need to be reminded about the same issue twice.
Imagine you’re working in a factory and some people never put the tools back in their place and leave a mess at the workstation. Are you going to clean up after them or tell them to clean up after themselves? It’s the same with code.
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.
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.
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.
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
@@neociber24 Nit picks are minor problems by definition.
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.
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.
again, those can get lost, they can clutter the ackshual important work, and generally giving everyone the right to ignore them makes them meaningless.
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.
@@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.
>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.
@@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.
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.
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
@@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
@@matthias916
Spot on!
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.
@@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
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.
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.
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.
@@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”.
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
@@0oShwavyo0 you guys review commits separately? I thought people only review the entire pull/merge request and not the individual commits one by one.
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.
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?"
@@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.
@@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.
@@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.
99% of nitpicks belong to the realm of linters and formatters.
Edit: Commented before watching the full video.
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.
that's fucking terrible
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.
Single-letter variables for scopes which span a single line are totally fine.
Merge now, fix when a bug is found in prod
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
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.
Hm, that way inconsistencies will accumulate.
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
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.
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".
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.
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?
I hate prefix-based development. Underscore, dollar sign, @ sign, I hate it all.
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.
100%
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.
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.
OMG dude, I've been here. So much back and forth
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
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.”
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.
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 ^_^
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.
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.
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.
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".
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?
as a junior dev, Me liky when people nit picking my shitty code.
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.
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.
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.
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.
Except in this case you are correct and they aren’t. Early return is paramount, and deeply nested code sucks.
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.
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.
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.
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
That said, anyone writing elementList instead of elements should be chemically neutered and forever to work in the mines.
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.
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.
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
@@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.
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.
Why is it QA lead who made the call?
@@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.
4:46 is the reverse true? Is it a nit to get people to remove leading _ from private fields when they add them?
9:08 nitpick: you mean you have an X coming
I have a colleague that is super into "Clean Code", I avoid tagging him into reviews...
....hey Bob, I noticed in your last PR that your structs have comments above the parameter vs inline.... Did you get the memo ???
well done, but it should be rotating
@@raph151515 rotation is kinda loose, so I wait for the simpler PRs to tag the nitpickers
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.
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
“One’s ‘scrupulous critique’ is another’s ‘principles to cherish’, Let not the assiduous be misconstrued as the fierce.” - Me
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.
that isn't a nit, that can be a suggestion
@@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).
@@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.
@@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.
@@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."
Just wrote a whole package with my private variables with an underscore at the start... Gonna make a refactor PR right away 💀
Nice! Now I can link this video when they nitpick my code 😊
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
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
OMG YES This is the video I've been wanting you to make
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…
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.
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.
@@raph151515I would like to nitpick your comment but I have autism
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)
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!
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.
I'm on board. But can imagine lots of devs being against it, arguing "The problem isn't my nitpicking. The problem is your code is hard to review because of of tiny confusing details which draws the reader's eyes to them and prevents thinking more about the big picture".
this may have at least changed my career if not my life. thank you.
As a C# loser I'm really rooting for these guys to finally invent static code analysis. They are really close
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.
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.
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
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!
I felt that “C# loser” part. Gonna go check my LOGAF quick.
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 😂
I've had an hour + conversation regarding the use of == vs === in JavaScript because of the same issue. That conversation drained the hell out of me and we were left with the all so obvious point of, it needs to be == for this specific function.
let me guess, you had an item that was null or undefined and you used == null
@@ThePrimeTimeagen Not even that complex, just a situation where I'd personally choose the user experience over the code state, he didn't agree and this was his only comment regarding the PR, he hung on it like a tag nut on arse hair.
Basically my application had loose typing in the backend (it was my first JavaScript project), bug appeared in the front end after said developer blindly put === everywhere in the application. I attempted to push a quick fix for the users, we could clean up typing later it wasn't that dire to re-type it all straight away.
For more context it was an open source project I was working on in my spare time, so not... at all the place or need for nit picky code reviews.
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.
how do you prefix private variables if not with underscore? Good luck with setters and getters then
13:00 What he is describing is the conceptual difference between testing (minute) implementation details (bad), versus testing actually expected application behavior (good)
where do u draw the line? should i just click approve if the feature works?
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
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.
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)
isn't private modifier in typescript transpiled to non-private in javascript?
If one can offload all of the nit-pick stuff onto the linter, it solves two problems:
- immediate feedback, doesn't have to wait until code review.
- I'm not the bad guy, the linter is. (Furthermore, people _mostly_ don't take offense to linter feedback, they'll usually comply.)
Won't prevent all instances of nit-pick merge blocking, but it circumvents a lot of it.
Well said! From someone guilty of being a nit picker 😅 Also code formatters absolutely rock! They eliminate manual formatting bs and uniform coding style across the project.
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.
I think it depends on the team. Used to code with a bunch of codeforce grandmasters. They would nitpick. But defaut code they would produce would by default be about nitpick free. So overall, was an incredible experience. Depends on size of team and organisation , but if all the team can get to some level so that there is no reason to nitpick anymore, also a pretty good outcome
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.
🤦♂️and changing block level elements to inline is more than just a semantic change
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.
I do tabs for indenting, spaces for allignment.
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.
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.
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).
3 space indenter here... after changing to it I just see how much more lines up from line to line.
1 ex. if(blabla, the blabla will align with the first character on the next like because its in the if scope...
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.
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.
@@corruptedsave145 It shows you a list of all things you can get, isn't that what you wanted to do? Seems perfect to me.
get/set is not a problem.
It's a symptom.
Should just be x.time for getting it tbh, like any other object access
@@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.
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.
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
During my internship this summer, one of my CR’s had 6 revisions, all because of np’s
OH MY GOD I WAITED THIS VIDEO FOR SO LOG! SCREWWW YOU ROGERIOOO!
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…
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.
a healthy team is a willing to go against what you think and try something else. - ThePrime
"Don't shoot yourself in the foot!"
- The Primeagen