Very nit picky code reviews
Posted by SmartAssUsername@reddit | ExperiencedDevs | View on Reddit | 164 comments
I started a new job about 3 moths ago. I'm very used to code reviews since I've been in the industry for 15 years now.
The team lead is doing most of the code reviews and my god there are so many nit picks that it's driving me crazy.
I'd say 50% is legitimate stuff. 10% is questionable but fairly harmless, and rhe rest is just very nit picky.
I had 8 commenta about variable names on a 20 line commit.
At first I paid attention thinking, yeah, maybe my variable names were bad. Nop, just things like "emailsFromUser" should be changed to "emails". Which is like...I guess? Both are fine if you ask me. I like my variables more verbose.
It's making me question if this is the norm or not? I've worked for 3 companies my whole life, barring this one.
drizzyhouse@reddit
This is how I've felt for so many comments over the years. I've often asked, "Why?". Other times I've asked, "Is it better, or is it just different?". Lastly, "What is the team's agreed upon convention?". I've found them to be fairly disarming, and helpful at repositioning people to focus on things more important.
I also started prefacing my comments with my intent, with one being being something like "nitpick" or "thought", to convey that perhaps it's just something I'd like. I then saw a year or 2 later that someone had formalised this as conventional comments. I'm now at my third job in a row where at each one, many engineers loved the approach, and adopted it without any encouragement.
No_Structure7185@reddit
and what do you write if it should really be changed and isnt just a minor thing/nitpick/thought?
drizzyhouse@reddit
I use fix. Depending on your culture and process, Request Changes as well.
No_Structure7185@reddit
ah thats good, thanks đ will try that
metal_slime--A@reddit
I think I like your naming convention far better there.
Nitpicking is not uncommon though. I find it more prevalent with smaller teams with a smaller radius of idea sharing or exposure to external code bases..
Nitpicking is also kind of subjective. Some people take literally no care for any details and find just about any critique of their work nitpicky.
delventhalz@reddit
I would say it depends on context. Variable names should scale with the amount of code they exist in. If that variable lives in a five line function that is already called "handleUserEmails", then OP's name is redundant and adds clutter without adding clarity. If it lives for a hundred lines of complex logic, then having the extra info in the name is definitely appreciated.
nasanu@reddit
Clutter lol, thats a good one. I can use this to block so much code, its beautufly subjective and unprovable.
delventhalz@reddit
Obviously. Every readability discussion is subjective. It is literally about whether or not I, the subject, can easily read your code.
nasanu@reddit
Yeah and if I say I cannot read it, wtf you going to do about it?
delventhalz@reddit
Fix it obviously
nextstoq@reddit
There is absolutely no "clutter" in a name like emailsFromUser, providing it describes the use of the variable. I don't understand the comments about clutter at all
donnysikertns@reddit
You're totally right. However I found some devs simply lack the ability to see this bigger context and will always feel they are just being picked on instead of realizing what the issue is.
testsubject23@reddit
And alternatively, if it's a 5 line function that could be applied to any email, such as "extractEmailText", then specifying them as user emails adds potentially misleading context
Maxion@reddit
On the other hand, if this 5 line function is specifically related to userEmails, and the same data is named in other functions as emailsFromUser, or just
emails, one should most definitely continue the same naming scheme.Creaking_Shelves@reddit
This is the key!
mirodk45@reddit
I see this a lot when people get feedback like "you should critique more on code reviews" and people start to overcompensate like that
chevybow@reddit
Once had a manager enact a rule that if youâre doing a code review- the intro review needed at least 1 comment. But preferably much kore depending on size. Yes even for 1 line PRâs we always had to leave a comment before approval.
It resulted in very nit picky comments and a weird team culture. Their reasoning was that âno code is perfectâ.
Fun-Dragonfly-4166@reddit
i thought that was why LGTM was invented. i have never gotten a PR approved without at least one comment but that comment may be LGTM
Working_Apartment_38@reddit
Thatâs almost worse than using lines of code as metric
ShoePillow@reddit
It's the same category. Measuring things that should not be measured.
DogCold5505@reddit
Prefixing small or subjective notes with with ânit:â is usually appropriate IME (which means ânot blockingâ). Â I want to hear my teammatesâ opinions but none of us want to slow down unnecessarily.Â
doyouevencompile@reddit
Ideal codebase should look like it was written by the same personÂ
metal_slime--A@reddit
I've seen a few really far from ideal codebases that manage to appear to have been written by a single developer.
ings0c@reddit
The same 4 brain cells, in fact.
doyouevencompile@reddit
PâQÂ â QâP
metal_slime--A@reddit
Something tells me I'd hate to request code review from you
reyarama@reddit
Probably cos you can't think logically
eggrattle@reddit
A nit-pick doesn't even warrant a response, or prevent a PR from being approved. That's why it's a nit-pick, they are subjective preference based suggestions that do not affect the performance, or delivery of a change.
I ignore 99% of them and rarely leave them because they're so useless. I'll generally leave them on issues that can improve a reviewers experience, especially for juniors.
It's a focus on form, when the PR should be focused on function.
One massive nit-pick I have is the use of emojis. As soon as I see an emoji my default is this is vibe coded AI generated slop.
Instigated-@reddit
Except when someone is being nit picky, but not approving your PR, so you canât move forward without making the changes they want. And the longer your work is held up in the approval process, the more likely youâll have to spend time dealing with code conflicts as the code base changes under you.
Imho being nit picky with juniors (or any subgroup) while letting things slide for experienced devs is a double standard that misuses power dynamics. The person on the receiving end can see other peopleâs PRs being approved with the same things, and the reviewer giving themselves a free pass, while the work of the junior is being held up for no good reason. Multiply it across the team and itâs like a stacks on. Many people feel entitled to nitpick X, while they basically green stamp the work of a senior.
bwmat@reddit
That's why I nitpick everyone
ravnmads@reddit
I do nitpicks, but I prepend with ânitpickâ so that people know that it is below âsuggestionâ. This way they might use my comment or they might not.
Pretagonist@reddit
I do the same. It's more like I would have done it like this but your version works so if you want to keep it that's fine too.
I won't block a PR over nitpicks
Own_Candidate9553@reddit
It's tough - you can find some outright wrong/dangerous stuff in code reviews, but outside of that it's often pretty subjective. My personal stance is to for sure call out important things, and beyond that I try to ask myself "is this thing I'm about to ask about actually incorrect in some way, or just not how I would have done it?" After you do this awhile it gets to be habit. And I'll often put "Nitpick" on a comment if it seems trivial.
This seems like a learning opportunity for the reviewer. It's not actually helpful to weigh in on every decision the coder made. The variable name was fine, didn't need addressing. It's a waste of everybody's time.
Depending on the politics there, I'd try casually talking to others on the team, see if they are also annoyed by this one dev's reviews. See if you can start a conversation in a retro about what the team is looking for in reviews. Maybe even come up with team standards on naming and stuff. If most of the team is on the same page about stuff it's easier to push back on nitpicking.
If none of that is possible or practical, talk to the manager. Not in a harsh way, just "I think X could benefit from some coaching on good review practices." You could help gather examples. Having examples from multiple team members would be best, to show it's not personal, just a waste of the team's time and energy.
Steinrikur@reddit
Conventionalcomments.com is a nice template to follow when making comments.
Nearby-Middle-8991@reddit
for me, it was someone senior in title (only apparently) saying an operational issue was "style choices" as excuse to not fix it. That's usually a red flag, if even before getting to the comments they start making excuses. Or when they leave the branch sit for like 2 weeks and then open the PR with "sorry for the short notice, this needs to be in today"...
metal_slime--A@reddit
Approval via coercion. My biggest red flag of all.
margmi@reddit
Our rule is that nitpicks should start with âNIT:â and are optional.
couchjitsu@reddit
I used to follow that rule. Then, not much later, I decided if I'm starting something with "nit:" then it doesn't need to be said at all
Pozeidan@reddit
It seems like a good idea at first because it reduces friction.
It depends if the nits are really opinionated or if they are actually improving the readability and consistency. At scale (multiple devs) for an extended period or time, it's possible you end up seeing a degradation of some aspects of the codebase which isn't great.
However too many nits and you lose the purpose of the reviews.
It really depends on the codebase. For a legacy system full of crap it's probably not worth it. For a new-ish project / product you benefit from the added quality for a very long time potentially.
thekwoka@reddit
also a difference between someone new on the team being informed of expected standards, and someone that has been there a long time that generally has very few of those "nit picks" and in places where the "violation" is more reasonable.
Kind of like a "this rule individually isn't important, but in mass is, and you gotta learn the rule to know when to break it"
Pozeidan@reddit
Absolutely!
matthkamis@reddit
Well, what if the person reading it learned something, and agrees with the change?
Sparaucchio@reddit
"Learned something" very important, for example that you like to write numberOfThings instead of thingsNumber...
Either this stuff is defined in a guideline, or it is useless waste of time
Creaking_Shelves@reddit
nit: thingsCount
Sparaucchio@reddit
Exactly. Either this is defined in an official guideline, or you'll end up with 2 different devs giving you 2 different nitpicks in 2 different PR reviews. Because one likes "thingsCount" and other likes "thingsNum". Ask me how I know... Absolute waste of time.
But judging from the downvotes, people here like this kind of shit
Steinrikur@reddit
I think y'all should look into https://conventionalcomments.org
Tldr: start your comments with keywords so that the priority of them is clear
Fidodo@reddit
Exactly. I want everyone on my team to feel comfortable providing and giving feedback. Just be clear about what you really want changed and what's a stylistic suggestion.
Sus-Amogus@reddit
I appreciate the nits. Helps me align better with the team and learn.
felixthecatmeow@reddit
I still leave those for stuff that I know is gonna annoy me every time I look at it but I'm not gonna want to add a bunch of unrelated lines to my future PRs to change it. Like spelling errors in variable names.
gajop@reddit
Spelling errors are small issues but I expect people to fix that. That's far less subjective than stylistic things.
thekwoka@reddit
typos in variable names can be caught by tooling.
WanderingThoughts121@reddit
Not sure I agree with that, A lot of time when I leave nits on variable names or the like I get feedback from the author that they like that better because it expresses the concept slightly more clearly.
I use nit on stuff where I think x would be better but if they disagree itâs not something I will block over.
thekwoka@reddit
I think they can be more important on newer devs to the code base, since it's about establishing some standards that maybe aren't too important individually, and they need to learn them.
eggrattle@reddit
This. Right here is the way.
Few_Raisin_8981@reddit
Why revieycode against a style guide when there are linters an precommit hooks?
Covet-@reddit
Linters canât catch everything
ninseicowboy@reddit
Yup. Just say
nitSaykee@reddit
My guess is the tech lead doesn't have to write any code that interacts with "emails" and won't forget in a few months when he goes back to work on it that they are "emailsFromUser"
More clear the name, the easier someone's job next time is đ¤ˇđť
If I just had a variable called emails, I'd end up writing a comment to explain what emails contained so someone else would know later! More work/code!!
UntestedMethod@reddit
Meh. Different teams have different standards and conventions. You've been there 3 months so you should be more in "observe and adapt" (yin) mode and less in "push your opinion" (yang) mode.
If you truly "agree to disagree" then it should be NBD to apply and comply with your new team's standards and preferences.
Btw, I do agree with your opinion on verbose variable names... But not every team or code base will require that.
Best to shrug it off until you've gained more political weight in the company and historical understanding of the technical situation.
superdurszlak@reddit
It really depends on the situation and sometimes there's nothing working with being more "yang" as you put it, often it's beneficial and sometimes it's even necessary.
To give one example, we had a new team member a few years ago who clearly had strong opinions (which he sometimes expressed in rather unwelcome ways). We could go defensive and put him down, but as I was his "buddy" I noticed what he tried to communicate was actually valid, and while I asked him to communicate in a less aggressive manner there were good reasons to implement his suggestions - to give one example, we ran multiple instances for HA rather than scale, but somehow none of us had ever come across the idea to set up anti-affinity on our pods to make sure they were not running on the same node (often they were). He was able to explain why we need it, we accepted it and it was beneficial. A defensive team would teach him to "keep his head low" instead and never be willing to take a suggestion that something could be improved.
gajop@reddit
I wish more new employees/contractors come with this in mind. Being in a place for years to see some new guy come and try to enforce his views on everything... It's very annoying to deal with, and I've experienced it way too frequently.
Even if you're on a higher "level" than those already there, you should first observe and understand the current practices before you start pushing your way in. With time you'll identify actual pain points that are worth arguing about.
cgeezy187@reddit
+1 - it happens, teams are different. I like this conventional comments chrome extension for GitHub reviews: https://chromewebstore.google.com/detail/conventional-comments/pagggmojbbphjnpcjeeniigdkglamffk?hl=en
SmartAssUsername@reddit (OP)
I actually like this approach very much. Good comment.
UntestedMethod@reddit
Cheers. I was also going to add, for anything you disagree with, try to find out and understand the reasons for the other's opinion.
Isollife@reddit
In my experience there's usually an inverse trend where the more nit picky a reviewer is on a line by line basis, the less that reviewer is thinking about the functional impact of the code - which are far far more important.
I'll generally scan the PR to check it's not complete garbage. Usually be more thorough line by line if I don't trust the engineer (I don't mean that in some toxic way, just if they're on the more junior side mostly). Then I'll be thinking about what impact is this actually going to have on the product and focus most of my attention there.
OceanTumbledStone@reddit
Try to get the company to switch to conventional commits
https://conventionalcomments.org/
Give categories for each message. Theyâd soon get bored writing nitpick for each one
superdurszlak@reddit
Never been a fan of conventional commits.
What really helps is communicating clearly, avoiding aggressive language or personal remarks, not prefixing questions with "question:".
OceanTumbledStone@reddit
Yea, although in my experience people can find it hard to transpose subjective guidelines onto their own comments. With a framework that levels everything, passive aggressive vibes get reduced.
Aggressive language or personal remarks should be a management chat, thatâs never acceptable
olionajudah@reddit
A single gatekeeper is already a deep red flag
ughthisusernamesucks@reddit
By chance is this new company a fairly large company?
I've seen this kind of behavior before and it was always at larger companies that had performance evaluation criteria that included nonsense like how many code reviews you participated in and shit like that.
loptr@reddit
Liking something doesn't matter by itself in this context.
What does the rest of the codebase say? If your naming deviates from the rest of the codebase, the input could be valid without being a matter of "liking" one over the other.
Maybe. But probably best if you don't. If your disagreement leads to deviating coding style within the project it's just creating incoherence for no reason.
But at the end of the day, the team needs to be aligned. Do variable names matter? When do they matter? What should they look like? These and more should be clearly documented if they are an expectation, and not just sprung on people during review/as they come up with them.
So have a team discussion about what conventions to follow, and/or ask for them.
PredictableChaos@reddit
Is there a style guide that calls these out anywhere? Is the rest of the code generally written using less verbose variable names (using your example)?
The reason I ask is that consistency is super important in a codebase (imho) and that may be what the TL is doing. If they're not being rude about it then I would advise you to stick to the existing style. Sooner or later you'll find yourself doing it by default and you won't get as many of these nits in your PRs.
This has happened to me before and I got pretty annoyed at first but then I realized as I got better at conforming to their style I didn't get hit with as many of those. The consistency really helped when I had to read parts of the code base I wasn't familiar with.
Personally I think emails is probably better than emailsFromUser unless you have another list of emails in that routine and it helps keep their purpose separate.
SmellyButtHammer@reddit
Agreed. On a new team you have to fall in line with a different coding style and that takes time. Totally agreed that eventually you'll get used to it and the nits will stop.
Consistency is important, it's always terrible to walk into a code base and all of the code looks off. The lead looks like a jerk for the comments but (s)he has a standard.
SmartAssUsername@reddit (OP)
I think you're both missing rhe point on this one. The whole idea is that it's very subjective.
It's like saying you're wrong for liking vanilla flavor over chocolate.
Altruistic_Yellow387@reddit
They were asking if the company has a coding style defined (from your answer it seems like no but it was a valid question. Lots of companies make up their own standard for everyone to follow for consistency)
Neverland__@reddit
You donât understand what a style guide is?
Honestly if you fix up the âlegitimateâ 50% of comments maybe they will all disappear?
thekwoka@reddit
Yeah, there is definitely an easy trend to fall into with a review where some clearer issues then make them look closer at small stuff.
But also, with new team members, you need to get them on the same page with a lot of smaller things.
Neverland__@reddit
Been going through that right now. A lot of back and forth on convention but worth it long term
WoodenPresence1917@reddit
The current code has no consistent style....? That's odd.
SmartAssUsername@reddit (OP)
I mean there code style for variable naming since...well, don't even know how you'd implement that. Or if it's even possible.
It's like policing language.
MocknozzieRiver@reddit
Huh? My team had guidance for variable names. You just like... Make rules together as a team and document it...? Like, "I think functions names should start with a verb," and then we'd agree or refine it.
WoodenPresence1917@reddit
So the variable names are chosen at random as long as they satisfy the language requirements for variable names?
SmartAssUsername@reddit (OP)
No. They're chosen to best reflect what they are. It's the "best" part that's called into question here.
WoodenPresence1917@reddit
Right, they are not chosen at random, they are chosen to fit into a certain style, one which you seem to not be accustomed to.
It may help to ask for the reviewer to explain comments you don't understand; I doubt they're just doing it to annoy you.
SmartAssUsername@reddit (OP)
Thankfully I've already done that and the answer was, and I quote: "This naming reflects what it does better than the other one".
I've resigned myself to just doing whatever the comment says, but it's very annoying writing code and thinking "how would my TL like this variable to be named".
Yes, I too find that statement ridiculous.
WoodenPresence1917@reddit
You could resign yourself to that, or you could say something like "sorry, I still don't understand and would like to get this right in future. Is there a resource you would recommend for me to learn more?"
Alternatively you can just seethe on Reddit for no reasonÂ
SmartAssUsername@reddit (OP)
Both is an option. Seething for no reason is a good way to blow off steam.
Almost like 2 different views can be equally valid.
Alternatively I could also be passive aggressive on reddit for no reason, seems to be working for you.
WoodenPresence1917@reddit
Very ironic response to genuine advice, good stuff đ I am sure your appraisal of what is genuine criticism in a pr is just as even handed đ¤
6a70@reddit
To say thereâs no coding style to be enforced here is missing the point.
My naming guideline is âas concise as you can be while still staying as precise as you needâ
Variable naming is dependent on the scope - what context is inherent/assumed based on where the declaration happens? What ideas are being hidden by omitting precision, and what ideas are revealed? Evaluate how your naming fares given its context (and the ideas you want to ensure are conveyed), and youâll find that you can fairly consistently evaluate names relative to each other
SmartAssUsername@reddit (OP)
And I'm not debating that, I'm saying your understanding of the context may be different than mine, and both can be equally valid.
6a70@reddit
we differ here. This is part of what I'm trying to convey: if two engineers have different understanding of the context, that means someone is either bringing in unwritten assumptions or has a misconception. Both indicate that maintenance will be more difficult than it has to be.
Both can be equally valid in terms of technical correctness, but they will definitely differ in maintainability. And in my book, more maintainable is more generally valid.
thekwoka@reddit
True, but the teams standard wins out, not the new guys.
thekwoka@reddit
And?
What does that matter?
The subjective standards of the team and code base are the standards, regardless of how subjective they are.
If you don't like this feedback, don't come to reddit just to look for people to tell you "yeah, fuck that guy"
Professional_Mix2418@reddit
Totally agree. I value consistency in style. Thus whenever in a team, be it as the lead, be it as a consultant, heck even as the CTO. Having documented guides for naming conventions, styles, and everything else is very important. Even more important that the team makes it themselves and are aware that there is a process and a way to change it as well.
It helps with general readability and also with code quality and even as one can see here with feelings of team members to prevent its being seen as nit picking.
Simply the sign of a team with a certain maturity level.
Michaeli_Starky@reddit
I'm a technical lead and an SA for multiple streams (teams) on the current project with about 30 developers (in those teams only). And I can say that people often exaggerate the importance of consistency in the code base overall. Yeah, of course there should be a baseline, but here no one in their right mind would block MR just because they have an opinion on the variable name. That's unnecessary, counterproductive and simply annoying.
Conscious_Support176@reddit
This isnât anything to do with verbosity. You canât understand why FromUser might not be appropriate? Hint: in the code that uses that variable, is it the fact that the email is from a user relevant?
There is nothing subjective about it.
CautiousRice@reddit
Just feed that garbage feedback to Cursor/Claude Code and let him have it, this is what I do when I'm attacked by a hostile code review.
It's very unprofessional by your team lead and shows immaturity and lack of experience.
NaverCZ@reddit
Depends⌠I once had a colleague that was like that.. I remember having a PR that literally just changed a dependency version number on one line and nothing else - received two commentsâŚ
If there are PR guidelines and PRs can be done the same in the other way around (ie you or someone else can do a PR to this guy in the same style as well) then it can be fine. In my case it was always mandatory from his side but he ignored comments from others and was overall just gatekeeping and creating a bit of a toxic environmentâŚ
Fingers crossed its the first for you đ
airhome_@reddit
That sounds soul destroying. I can't imagine many better ways to take the joy out of writing software.
A practical question. Can you start feeding the code review comments into an llm to synthesize a style guide that you can automatically run your prs through with an llm to flag issues before they go for code review?
It might actually work and let you effortlessly implement his arcane preferences without having to update the neural net in your head with someone else's rules. At a minimum you would find out if his preferences are highly arcane but stable or he is getting himself into recursive loops.
Far_Archer_4234@reddit
Im old and experienced enough to hold my ground on bullshit PR comments. If you are going to withhold an approval because i didnt name the variable the same way as you, then YOU can explain to stakeholders why we didnt meet our deadlines.
Hint: stakeholders dont give a crap how the variables are named. You held up a milestone because of your ego. Please clean out your desk and well have security escort you out.
ClassicJealous9410@reddit
Haha I laugh when old heads get downvoted. Like how dare you not acknowledge that engineers aren't delicate geniuses. You think I do this for money or to appease a business? It's all about the vibes brah'.
moyogisan@reddit
Can you get them to put (blocking) (non blocking) or something? https://conventionalcomments.org
roryg2025@reddit
This is best way to avoid ambiguity in pull requests. I leave lots of nitpicks on pull requests and I acknowledge that some of them are subjective do they all get marked with non-blocking so the author can make their own decision.
ClassicJealous9410@reddit
Is the review from multiple people or a person. That's an important difference. If the team consistently makes this type of comment, then they have unofficial rules/ style guide lines; that should be documented. If it's a single person, you can politely tell them to fuck off by pushing back. "I see what you are saying, but I think the verbose name blah blah blah...". If they want to have a whole conversation about your variable name, I would seriously question if you want to work there for the next X years.
aeroverra@reddit
Weird I would be advocating for your way. I can't stand nondescript variables
ValiantTurok64@reddit
You're still doing code reviews? I just let the linter and some agents do the reviews, I just read the summaries ...
aviboy2006@reddit
Until unless naming conventions is giving clear understanding no matter how we name it. Main purpose should served.
Gullible_Sweet1302@reddit
Heâs teaching you his preferred style. You are not taking the hints hence the recurring ânitpicks.â Take a hint.
nasanu@reddit
It's quite normal, but for me they aren't nitpicks, its marked as needs work and blocked. There have even been times when I have just caved and change to the wim of one demand only to have a different dev mark as needs work a second time because they want the same thing changed to what they prefer.
And it's all bad practice. There are rules for PRs, very few people know what they are.
SeriousDabbler@reddit
Some folk are more inclined to give feedback than others. Personally, I tend to think some restraint shows some maturity. It's possible this person hasn't been a lead for very long and is still trying to make sense of their role. Typically, I counsel my seniors to avoid giving feedback about superficial bullshit like this. That said, some people won't be told
theSantiagoDog@reddit
It depends on the person and the culture where you work, and honestly sometimes your relationship to the reviewer. I've been the victim of what I'd call the personal vendetta code review.
SmartAssUsername@reddit (OP)
It certainly feels like a vendetta sometimes.
Maybe I should compare comments on my code review to comments left on other code reviews. See if yhe theory holds any water.
Whatdoesthis_do@reddit
Personal vendetta code reviews are real and exist. I can know.
But in my expirience, this comes down to company culture. And culture is very, very hard to change.
TopSwagCode@reddit
First rule of code reviews is to have the rules clearly defined. Preferably in the code base / its own report, so its clear. Nothing worse with inconsistent reviews and rules there is in the tech leads head.
Second rule is to have reviews automated as much as possible. Have coding style inforced by tools and code quality too.
Its totally fine to have 1000 nitpick in a review. As long as they are cleared marked as nitpick.
Brilliant_Law2545@reddit
Moths ago. Changes requested. Denied
writebadcode@reddit
Fix the 50% of legitimate stuff before you submit the PR and you might find that theyâre less concerned about the nits.
If 8 comments about variable names is 10% that means youâre getting 40 comments that are legitimate issues. Itâs totally unreasonable to submit a PR with that many issues. Thatâs just completely unacceptable.
Just_Chemistry2343@reddit
Donât get into back and forth schedule a meet between you, him, and your manager, and get it sorted.
_lazyLambda@reddit
As funny as it may be to admit i am one of the nit pickiest mfs ever.
I have become the very evil I sought to destroy
Nah but its funny cuz I agree with your variable name far more, verbosity scales
You probably need to respectfully ask "why"
My suggestion is set a meeting to discuss coding conventions. They'll either see the pushback and buzz off or agree with you or it goes the distance and you force them to realize its an agree to disagree.
I'll add that im fascinated that a nitpicky person is opting for shorter names because there are actually problems with that.
OutOfDiskSpace44@reddit
Not everyone does a code review in good faith. Take a step back and examine the meta at the organization. Is the team lead doing this for any other reason? I have had this issue before, in every code review there was nit picks about everything and it never let up. Every code review was littered with nit picks and he did that to every team member. His job wasn't to make the team succeed, it was to make it look as if he was doing his job. It took me months to realize that and to understand he wasn't operating in good faith.
You can be proactive and ask a few times about variable naming and the usual nit picks before going into the code review. Usually they'll back off.
And you know what they say...the smaller the merge request, easier to review, more comments on it. 2000 line commit? no comments, LGTM.
behusbwj@reddit
Or you can follow the team leadâs lead, and let them set the direction on subjective topics to keep the codebase consistent
Hot-Hovercraft2676@reddit
I never know how the naming of a variable has anything to do with the quality of code. Most people claim that nit picking is for improving code quality, while in reality, its about two options, which are 98% and 99% good enough and most of the time just an excuse for forcing people to do something in the way you want.Â
For the naming problem, you see the comments here are 50-50. Why on earth do you waste time arguing about that, claim yours is better and therefore the PR should be blocked then?
gpfault@reddit
Write less stuff that'll get nitpicked? You're working inside an established code base with a team that seems to have preferences that are different to what you're used to. Adopt their practices and move on. Even if your way is "better" by some metric it doesn't matter when you're the only one doing it.
MonstarGaming@reddit
I'll start by saying no that doesnt seem normal, but if if the nits are marked accordingly then its not a big deal. That being said, I'll die on the hill of good naming conventions. Nothing gets under my skin quite like nondescript names or names that try to be descriptive but are actually wrong.
cannedsoupaaa@reddit
You've just started - have you read the code base to understand the style in which it's been written? This may just be a symptom of not being on the same wavelength. If that's the case, it's going to be more of a problem than just nit picks.
josetalking@reddit
Is it a variable or a property in a "contract"?
If it is property in a contract, I will 100% nitpick on names (not necessarily in your example as I need more context to know which version I like better).
If it is a local variable, as long as you are in the correct ballpark, and it is not misspelled, I don't care.
Whoz_Yerdaddi@reddit
You're in the right here. "code as prose"
brava78@reddit
If someone's most frequent concerns in code review are stylistic nitpicks, then they really need to find new things to worry about.
Code review must be primarily aimed at preventing peers from unknowingly breaking production, or committing a grave architectural mistake that will cost them extra work in the long run.
If engineering time must be spent on stylistic nitpicks, then the team is likely not prioritizing right.
Logical-Idea-1708@reddit
Sometimes, I just think itâs the tech lead asserting dominance. So much of this stuff is unnecessary. Are people going to be this nitpicky when all we do is vibe coding?
Shinobi_WayOfTomoe@reddit
I feel this pain. There are certain people on my team that just love to hold my PRs up. Itâs not necessarily nitpicking but questioning every other decision made, most of which turns out to be completely fine with a few instances of legitimate change needed. I just say fuck it, if this is our process then donât expect me to be churning out feature after feature because of all this red tape.
HoneyBadgera@reddit
Use âConventional commentsâ
lucky-Vi-king@reddit
We all know that guy!
keelanstuart@reddit
I had a situation like this... was kind of a dream job... but the guy I was dealing with was awful. I left after 3 months when I got a call from my old boss asking me if I would come back. Upon leaving, I had two other engineers contact me to say they completely understood... one saying "you know what got him to leave me alone? when you joined!"
Guh.
It happens. He was ex-Google and had people in his influence - but he was terrible.
ReflectedImage@reddit
Yeah, you will occasionally run into companies like that. I used to have a script called "translate_into_ancient.py" that went over my PRs and did all the weird things. You can probably create one yourself.
reboog711@reddit
At my current position; "nits" on a PR are very common. Does your team have a documented standard for naming conventions? That may help.
It is very difficult for me to create succinct, yet detailed, names.
dragonfleas@reddit
nit picks should be enforced by the linter
PickleSavings1626@reddit
eh, tried that with tflint/terraform. every single folder would've needed to be fixed and the smallest change would result in "please fix these 100 other issues". i still want to comment my nitpicks but i just hold my tongue cause i believe the same thing. enforce with code.
dragonfleas@reddit
i have a couple thoughts on that, first of all, lint rules should _always_ reflect what your teams practices are first and foremost. You and your team should be setting these as constraints for everyone, if you want a new constraint to be set, yes sometimes that involves going and fixing all of the violations. Tools like eslint and golangci-lint are excellent mature tools that support a variety of different types of enforcement.
You can do incremental adoption of linters, disable everything, enable things that are least disruptive first, and incrementally more and more rules. The alternative is set all of the rules that your team _wants_ to follow, and make it non-blocking, so that _new_ code introduced will match the rules set forth by your team, and old code can be incrementally fixed over time.
I'll weigh in on the _Terraform specific_ reasons why you may be hesitant to use a linter and that's because `tflint` is extremely immature compared to other linters out there. The tooling ecosystem for terraform is weak overall but I, and many others are working on resolving those things with opentofu
https://github.com/opentofu/opentofu/issues/2213 go upvote this if you want better linting
EvandoBlanco@reddit
There's not much to do in the moment, but it would be a good bet to just write down some egregious cases. Then it can be a "hey is the X dollars your paying to have me change emailsForUser --> emails worth i?"
softgripper@reddit
I'll only question variable names in a PR if they are really out of line.
However, some feedback on the
emailsvsemailsFromUser, if you are in the middle of some function and it's obvious that these are emails from a user, use the simpler term.When you do this, it's easier to spot reusable or similar patterns that can be extracted.
If you're in some context that has 2 different collections of emails, by all means differentiate.
What I've found - repeatedly over the years, is people over complicate variable names, and missing cleaner implementations as a result.
I get it, naming stuff well is one of the hardest aspects of the job.
Fidodo@reddit
In my team we just note what's a nitpick and what's optional and what's subjective. I don't want anyone to feel like they have to hold back feedback but I also want the feedback contextualized.
zica-do-reddit@reddit
Is the whole team doing this or just one person?
circalight@reddit
This is unfortunately where subjectivity comes off as objectivity. It can be maddening. Detach if you can.
hangerofmonkeys@reddit
Heh, staff have stopped nit picking PRs now that Codex and CodeRabbit do it for us.
Your mileage may vary.
armahillo@reddit
Does your team have an automated linter?
Does it have a coding style guide?
Automated tools help a lot with resolving these kinds of grievances since they provide a neutral DMZ for this stuff
Arbitrary nits like this arent a good use of anyones time.
Stock_Blackberry6081@reddit
It's a real problem. The best tech leads are people who would never want the job of tech lead. The worst ones are the people who are eager to take on the role.
PageSuitable6036@reddit
Have you talked to them? I think thereâs a spectrum for everything, and without knowing specifics, itâs difficult to judge the situation. Itâs really tough to work with someone who pushes unreadable and unmaintainable code. It can also really slow progress when people make too many comments.
In my opinion, the best decision is always to just talk to them about it with an open mind.
âHey, just want to check, I have this deadline and these nits are kind of slowing me down. Do you think I should follow them all? Or are you just earmarking them to help us both grow?â
I personally leave a lot of nits on PRs primarily when I think itâll enhance readability down the line. I think of it almost like proof reading an essay. Do I expect everyone to follow them? No, and I will tell every person that joins my team that on the first PR I review for them.
I think if you believe your way is better, then ultimately it should be your decision, but itâs important to at least consider their point of view. But if you never talk to the person, your resentment will just grow
moreVCAs@reddit
see if they would be willing to adopt some version of https://conventionalcomments.org/
In my workplace, we have a very high quality bar but also a general desire to move things forward. Youâll (almost) never see PRs blocked for âno good reasonâ.
HOWEVER, itâs still always helpful to calibrate to how much a reviewer actually cares about each comment. If people start marking all of the nitpicks âimportantâ, you have some kind of impedance mismatch. If they mark them ânitpickâ, youâre aligned.
ParsleySlow@reddit
I got pinged several times for too much white space recently. I'm old, so I just rolled my eyes and got on with things. They'll have a lovely looking code base, and no features that customers want. Good job.
CardboardJ@reddit
If I'm being nit picky I write 'Nit: this name is kinda meh'. I'll still approve a PR with nits though.
lokaaarrr@reddit
Same
lokaaarrr@reddit
Document standards, enforce as many as possible with a linter / formatter. Make code reviews about design, logic, correctness, etc
ballpointpin@reddit
I had a reviewer like this who did an amazing job finding problems in code reviews, but 80% was just nit picks about formatting. I ended up writing a simple linter, called something like PaulLint that I ran before submitting any PR. He was so happy, and could devote more of his time finding real issues. Win-win IMO.
washtubs@reddit
It's good to take feedback with the assumption that there is a real value system that they're trying to enforce.
Every MR, among those nitpicks don't fight anything except one thing. Try to dive deep into something and actually have a real conversation about why we want it and how / if this can be part of a style guide so this idea can be collectively enforced. Why does he not want
fromUser? Is it just because it's just too long? Or is it because he thinks it's more general purpose than you're envisioning. IMO we often do ourselves a disservice by labeling our comments nitpicks because they can reflect some important differences, despite an innocuous substance.I guess the main thing is try to see logical throughlines to the comments and don't ever assume they are whimsical unless you've just given up. It may even be whimsical but when you show them that you're trying to make sense of their comments in a generalizable way, it might pressure them to think more carefully about their comments in the future.
tom-smykowski-dev@reddit
The norm is that it varies between organisations, teams and even team members. There's no sharp line what should be standarized in the codebase and what not. I was working for one organisation that used popular linter, had extensive coding guidelines, and on top of that around 400 company specific, unwritten code style rule selections. What helped me was Hinty, an extension I wrote https://marketplace.visualstudio.com/items?itemName=tomasz-smykowski.assistant . Because I got hints when something broke rules. Adapted it cross team and it worked great. Another option now available is writing down these rules in your IDE AI settings. If you don't want to use AI to generate code, you can just ask it to format code according to the unwritten and written rules.
besseddrest@reddit
has the nitpicking been brought up in a retro mtg? I think if you could show that ABC tasks have sat much longer in review than reasonably necessary you can pinpoint cases of nitpicking as part of the problem. There may be others on the team who feel the same way as you.
bulbishNYC@reddit
I would fix the nitpicks and try to see common themes. A couple of more pull requests and you will be on same wavelength with the reviewers and realize the whole thing is a non-issue.
And you anticipate lots of comments, just get on a call instead and go over the code together.
edgmnt_net@reddit
Depends on the ecosystem and surrounding code. But more generally, uncalled-for verbosity should be avoided, e.g. if you have a sendEmail function dealing with a single email there's likely no need to disambiguate and call a variable emailFromUser or even emailFromUserToBeSent. Now, as for whether that's nitpicking, I wouldn't necessarily consider it useless. If code goes against surrounding style or best practices it's worth pointing it out at least. I've seen open source projects take this very seriously and it's often an alignment issue that's fairly quickly sorted out once you get used to it. But obviously you can disagree which complicates matters.
false79@reddit
I worked like at place like this. I too found it annoying but different environments will require you to adapt.
After I left that place to work at other places, I learned the quality of my code got so much better, compared to before.
At that point, I had 15xp in thinking I already knew everything.
Good PR comments will leave you shook to the core to do better.
PandaProfessional359@reddit
If I only leave nitpicks , I still approve. For me itâs a suggestion or possible a learning. Itâs up to the dev if they want to make the change.
Clyde_Frag@reddit
I've seen this from people that are trying to move to mid level or senior eng and are told by their manager to get more involved in code reviews, but instead of having the wisdom to only offer useful advice they nitpick things that don't really matter that much.
6a70@reddit
what specifically are you bothered by? That your PRs arenât getting approved? Or that youâre getting nitpicked at all?
CheeseNuke@reddit
Nearby-Middle-8991@reddit
As someone who reviews PR for a living. There's nitpicking and there's nitpicking.
What I try to do is also include the reason for the comment in the comment. In this case, 'please change emailsFromuser" to "emails" because style guidelines .... ' not just vibes. It's an extreme example tho, I wouldn't complain unless it was inflamatory or obscene.
Something you could do is ask "why" for the changes where the rationale isn't clear, if nothing else so you learn the pattern and don't do it again. That's also why I always check other people's PRs, including closed ones, when I join a new org, just to see how it happens.
Evinceo@reddit
Sometimes it's a hazing ritual to get you used to a new coding style. Sometimes it's a particular reviewer you need to learn to work around. Sometimes they want to be doing the task and are using you as a meat puppet to do it.