How strict should code review be?
Posted by Leather_Opposite_452@reddit | ExperiencedDevs | View on Reddit | 183 comments
My colleague’s verbatim attitude toward code review is that “every line of code should be scrutinized on whether it could possibly be improved”.
This results in addressing A LOT of feedback items. 90% of which have 0 impact on the performance or outcome of the code.
I’m curious whether this is the right way to do it or not. Intuitively it feels wrong to have to spend that much time on such feedback.
ImSoCul@reddit
not the question you're asking but as reviewer you should be highlighting which changes are "must do, security vulnerability" vs "this is good to address now" vs "nitpick" vs "personal preference" vs "learning opportunity here". All are fair game as long as they're clearly differentiated so the author can decide which suggestions to incorporate, skip, or come back to later.
For your question, if it's enough to piss you off, then it's either poorly conveyed, overkill, or possibly your code could just be really bad. If there's a lot to address, I'd just ask for quick call with the reviewer to walk through together. People are (usually) much more lenient and amicable when speaking vs just looking at code
Leather_Opposite_452@reddit (OP)
I am open to the idea that my code is really terrible. However, I am not the only person experiencing this and we have also agreed in the past that 80%+ comments that are left have no impact on performance or outcome.
nicolas_06@reddit
Why do you care so much on performance ? Shouldn't it be more maintainable and correct than performance and outcome ?
Leather_Opposite_452@reddit (OP)
When I say performance and outcome I am referring to the fact that the code does what it is supposed to do with no bugs, the comments are not related to this.
Yes maintainability is naturally important I did not suggest otherwise. However I think there is a standard of maintainability / readability which is “good enough” and that 30+ comments trying to reach optimum maintainability / readability is a waste of time
Leather_Opposite_452@reddit (OP)
What I am trying to say is that there is a difference between “is this code maintainable?” And “is this code as maintainable as it humanly can be?”
nicolas_06@reddit
This doesn't make any sense äs it humanly can be". My sister work for embedded software in planes. Think like auto pilot.
What they do for that is they have 2 team implementing the same thing in 2 different programing languages and that they cannot share or discuss with each other. In the plane 2 autopilots are available and if the first one fail, the second one is used.
The operating system, the programming language have to be certified too as well as everything else. So typically that's ADA, assembly or C. Object oriented is forbidden as too risky. Network protocol like TCP/IP are not guaranteed to transmit the info on time and can't be used.
So they reimplement everything including their own data structures.
The code need to have 100% code coverage. Each line of code has to be justified and be linked to a formal requirement. People will audit the code and a failure to follow the process means the code is not accepted and you have to do it again.
This is completely humanely feasible and hopefully as otherwise we couldn't not use any software in any critical application like health care, embedded device in a car or plane or spacecraft otherwise.
But it is overkill for non critical applications.
thekwoka@reddit
Oh, that's why these planes be crashing then.
Why don't these systems use Rust?
nicolas_06@reddit
I mean get rust certified, we will look at you doing it.
thekwoka@reddit
That's something the certifying body should be doing. Instead of holding back progress.
nicolas_06@reddit
apparently it’s on going but not by the certifying body. and yes because it’s safety first and we want to avoid issues like with boeing, usually you have like 20 years of delay before a new technology is adopted.
When regulation become too lax, that’s more deaths we get in return.
newyorkerTechie@reddit
If it’s something I know I’m gonna be working in for a couple years, I sure as hell nitpick it for maintainability.
jeremyckahn@reddit
It's only terrible if it doesn't work correctly.
thekwoka@reddit
This is why processors are like 1000x as good as 10 years ago but your device still runs like shit
jeremyckahn@reddit
Maybe so, but devs that are perceived as gatekeepers are the first ones to be let go. You do what you gotta do to deliver business value and keep team velocity high.
thekwoka@reddit
Yes, but there is a point between "if it works poorly it's still good" and "it much be 100% perfect".
Since the "gatekeepers" can also be saving a lot of money by ensuring velocity can STAY high with less bugs.
jeremyckahn@reddit
When did I suggest that bugs were acceptable? Those are clear functional issues that need to be called out in review.
thekwoka@reddit
Tell me more about how you know every bug that will happen before it happens when you just let bad code that "works" through
jeremyckahn@reddit
You can't catch every bug regardless of how thorough your code review process is. That's just life.
Healthy team processes and habits like automated testing, QA, CI and rapid, regular deployments do much more for team velocity and product quality than an arduous code review process, in my experience.
thekwoka@reddit
That doesn't mean you don't try.
jeremyckahn@reddit
Who's saying I'm not? I specifically stated above that I look for functional issues during code review. I think we're aligned on this?
Hundredth1diot@reddit
Disagree. Code can be terrible because it's hard to test, hard to understand or abysmally slow.
For instance, variables, function names and comments written in Klingon.
jeremyckahn@reddit
These are all things that can be changed by another team member after the fact if they feel strongly about it. What really matters is that team velocity remains high.
merry_go_byebye@reddit
Remind me never to hire you for critical work
jeremyckahn@reddit
And why is that? My team I deliver great work.
Hundredth1diot@reddit
You would approve a PR written in Klingon?
jeremyckahn@reddit
Of course not. I didn't take that to be serious or good faith part of your question, so I ignored it in my response.
jeremyckahn@reddit
Additionally, I would consider problematic performance a functional issue.
thekwoka@reddit
I mean, maybe you're all terrible.
ImSoCul@reddit
if you can non biased examples, we'd be able to offer some perspectives and maybe alternative solutions (like linting), but regardless of that the subtext is you don't want to deal with the suggestions so quick call to find common ground is best way forward
Leather_Opposite_452@reddit (OP)
Done here https://www.reddit.com/r/ExperiencedDevs/s/jySyw9Hawx
It’s largely not lint-able stuff. It’s a lot kind of optimization not in terms of performance but in terms of sharing every additional bit of new code with other areas of the code base. Making shared types, functions and components. Making code as shareable and as extensible as humanly possible.
ImSoCul@reddit
took a skim, some of those seem like valid suggestions. shared components in particular is a very good thing to point out in code review, that's not a nitpick at all.
The way I approach still is that everything has a cost, because dev time is expensive and costs the company real money. You don't even need to determine right or wrong, you simply determine how to spend your budget. If it's a small refactor that'll take me a few minutes, then I'd just do it. If it's a big refactor but also big pay off, then we might grab some more story points to address (or even address as a refactor/cleanup followup task). If it's high effort but low reward then just skip or if people are anal about it, make a ticket backlog it then never look at the ticket again.
I try to do some of this "budgeting" as part of my reviews, especially with junior engineers, but you'll need to push back on "spend". I have straight up left a comment like "hey I think refactoring this as X would be nice, but probably not worth the time rn. worth considering next time"
Leather_Opposite_452@reddit (OP)
I agree that shared components are absolutely good and not a nitpick.
What’s difficult to communicate however is the levels to these comments.
E.g there’s a difference between is this function shareable vs is this function as shareable as it absolutely possibly could be.
When you scale that rationale up to every line of code, every function and every component it gets extremely exhausting to address. Especially as each foray into increased sharing of components for example creates new opportunities to share more code which also gets commented on
jeremyckahn@reddit
"Personal preference" issues are not worth making a comment on. Everyone has different preferences and we need to learn to respect those of others.
ImSoCul@reddit
code isn't objective, almost everything has some degree of personal preference. I'm saying things like "python list comprehension" vs "for loop", those are technically both valid but in some cases one is easier to read.
jeremyckahn@reddit
To each their own. If the code works as expected and has no meaningful performance/security issues, I wouldn't comment on it.
thekwoka@reddit
so minified one liners?
thekwoka@reddit
list comprehension is never easier to read.
thekwoka@reddit
eh, depends, maybe toss it in for things like "I feel this naming could cause a conflict or confusion due to some other thing named similarly or that it doesn't seem to me to be the best way to describe what this is meant to do".
Sometimes it isn't clear that an alternative is DEFINITELY better, but it's worth positing if the person isn't super fixed on what they have.
lunivore@reddit
We have 3 levels of code review:
For urgent fixes - does it work, and how do you know it works? Does it risk breaking anything that can't be fixed (data migrations etc.?)
For normal PRs and following up those urgent fixes - does it improve the health of the codebase?
Nitpicks; preface with "Nit:" - any other feedback, should never block a PR from being merged.
Most of this is stolen from Google's guidelines.
Code doesn't need to be perfect; but the next steps to make it perfect should be obvious.
stevemk14ebr2@reddit
Nitpicks suck, I'm a staff engineer at Google and I hate nits. Half of them are just opinions, the other half are just wrong because they don't have context. All of them are annoying.
Venthe@reddit
A lot of things in code are "just" opinions. Good practices emerges from the discussions about the opinions. What you are saying here is that you feel too good to hold a discussion about the details that matter not for the computer, but matter for the people.
stevemk14ebr2@reddit
My point is a wall of nits is not constructive. People use the nit (sometimes) too frequently and it turns into a style debate. You interpreted what I said in the most extreme way
PixelsAreMyHobby@reddit
But he’s staff… I have met staff engineers who actually think they are better than everyone below their level. It’s about ego, power (and a bit more money). Ridiculous.
kuya1284@reddit
So true. I work with someone who can't stand when I try to provide suggestions on how to improve his code. For example, just today, I found four
forloops that could've been accomplished with one. In his mind, his code works, but in mine, it does so very poorly.redhq@reddit
I find nits are incredibly valuable for teaching me how my coworkers think about software.
Tokipudi@reddit
The whole point of nitpicks is that it is mostly based on opinions and therefore not important.
They should never block a PR, which is exactly why they are nice.
tim36272@reddit
Thanks for the advice! Signed, a flight controls engineer.
lunivore@reddit
My level 1 is literally "Does it work and how do you know it works?"
"Perfect" is not the same as "provably working".
We're talking things like "You used the old style of namespace" and "We like to use spaces after the brackets here". Your flight controls don't care.
Ironically many of the people leaving the feedback about the Nits were overlooking more important stuff like missing tests. Just bikeshedding.
Z0mbiN3@reddit
Perfection does not exist.
My philosophy teacher, on why he did not give out max grades.
asdfghjkl--_--@reddit
I have this one senior who would just do nitpick and unnecessary comment
Add final, use Objects.nonnull, unnecessary try catch for runtime exception that may occur
shozzlez@reddit
I mean, those are all reasonable things to comment on. Especially if those consistent patterns are already established in the codebase.
RicketyRekt69@reddit
This is the best way to do it. You don’t want to NOT be thorough but at the same time, PRs should not be sitting around for days over tiny details.
vivec7@reddit
Early on in my career, I loved those nitpicks. They're where I learned the most.
The only downside was I had a few seniors around me who were like this, and later joining a team that was very "tick and flick" made me feel nervous that my code was not being reviewed thoroughly.
I've since worked with some individuals who either saw it as a waste of time, or they would get very flustered by a large amount of feedback. It's not that hard to just keep communication lines open and tailor your feedback to that individual.
nopuse@reddit
This is the worst. A PR approved within a few minutes makes me anxious.
Comprehensive-Pea812@reddit
The only problem is when multiple seniors can't agree on a nit.
I would say it is helpful that a codebase has an owner to reduce decision fatigue.
yumt0ast@reddit
A lot of wrong answers in this thread.
This depends on your business.
Are you a multi-billion dollar mega tech co with 100,000 engineers, with a profitable product, whose goal is to make maintainable code for 15+ years? Yeah scrutinize the hell out of it.
Are you a startup who needs to launch a product in 3 months to determine if you even have something to sell? Code quality is lower on the list. Speed is really important. If it works, great. You can iterate and improve later.
thekwoka@reddit
Yup.
Is this code that will run on a pacemaker in someones chest keeping them alive or make sure the X-ray doesn't nuke someones insides? Scrutinize that to hell and back.
Some stay at home mom ecommerce side gig to stay busy? eh...
todo_code@reddit
You should have a style guide that everyone can point to, if there isn't something in the style guide it should be addressed outside the PR. Otherwise it's however a dev is feeling that day, and those kinds of nits are unhealthy for a project.
Leather_Opposite_452@reddit (OP)
The problem is that it’s not really style related per se.
It’s more like optimizing every single new addition as best possible with the existing code to the nth degree.
Is every test function testing everything as best as it possibly can
Is every component or function as extensible and shareable as possible
Is there a way that this new component could replace x old component etc
thekwoka@reddit
What do you work on?
Pacemakers or someones personal blog?
Leather_Opposite_452@reddit (OP)
I think there is a difference between is something extensible or readable vs is it as extensible or readable as it possibly could be
nicolas_06@reddit
Being readable is critical for most code that is not to be thrown away soon and will need to be maintained.
That the code is extensible for me is often an error and a non design goal. With 20 year of experience I can say that extensible code is the excuse for less readable, overly generic, difficult to understand, too fancy and over refactored code.
Most of the time, straightforward code that is not so extensible is much better and then when you need to actually to extend to code you refactor it to support the necessary evolution.
But 90% of the time, people make overly complex designs that do not help because there will be no extension or the new feature will be incompatible with the fancy design.
EkoChamberKryptonite@reddit
Uhh what? My view is these things i.e. extensibility and readability are complimentary. How can one easily extend a certain part of the code if it isn't readable and thus understandable? In my experience, most easily extensible code I've come across are pretty simplistic and clear-cut
Extensibility is a part of maintainability and in my experience code that isn't readable is not easily extensible, and thus hard to maintain.
nicolas_06@reddit
Readable to me is to straight to the point. The code does what is needed, the name of files/classes and function are clear and you avoid any fancy design pattern or architecture stuff that are not needed.
Most often than not, people make their design/archiecture overly complex to ensure it's extensible. The small monolithic service is cut into 3-5 smaller services, potentially each with its own git repo. Everything needs to be serialized and deserialized. Investigations become harder because you can't debug all things together.
Design patterns are added a bit everywhere because we might need to support this or that.
The code is made generic and less readable on top of being significantly bigger, slower and harder to maintain. Releases are more complex, it stress the network. Debugging is harder.
9 time out of 10, when somebody want to make the code more extensible, this is useless because the extension that will be needed is not what they made extensible. But the added complexity is still here.
The code should not be made extensible. The code should be extended elegantly WHEN the new features make it necessary even if that means refactoring it.
In the meantime keep it as simple as possible while still doing what it expected out of it with simple code that is easy to read and a very simple architecture and design.
Leather_Opposite_452@reddit (OP)
I agree that readability is important of course, but surely there is a benchmark of readability that can be achieved that is “good enough” vs trying to achieve optimum readability.
But yes what you mention is kind of the issue imo. A lot of the time is spent perfecting code for a future that never really comes.
Imaginary_Maybe_1687@reddit
To start off, no, every addition does not have to be the best it can possibly be. Because that's not what you are getting paid to do. This is something a Tech Lead should vouch for, but you do not provide code. You provide a solution. And everything you do should be measured as an ROI from your time vs value added to thr product.
If the time spent by the reviewer and the reviewee to produce and make a change is less valuable than that time being spent on something else, them the comment is not useful.
That is not an easy line to thread, but it is a good principle to keep in mind.
nicolas_06@reddit
To be honest, its impossible for us to know if your reviewer is too picky or if you are doing quite bad code that need lot of review... You only provide 1 side of the problem.
In all that, it's relative. The quality of code needed depends a lot on the context (do you do code for a plane auto pilot or for a prototype that will be trashed in 6 months ?). Is that code sensitive or not ?
And are the remarks good and pinpoint real issues or that they nitpicking. This is all relative again.
Maybe your reviewer is asking too much. Maybe you are not putting anouth care to your code. Maybe there a bit of both. I'd say often there a bit of both.
Leather_Opposite_452@reddit (OP)
I understand it is not possible for you to give an objective read.
However, that’s why I focused it on the rationale of the reviewer that “every line needs to be scrutinized to delineate whether it could be better in any possible way”
JWolf1672@reddit
Every single line should be scrutinized, for correctness, staying on pattern, obvious performance issues and readability.
Should it be scrutinized for every single possible improvement, no. That's overkill in many cases.
RelevantJackWhite@reddit
IMHO, a PR should only be blocked when the code is likely to cause a problem, or if it does not solve the problem it attempts to solve. I'm a big fan of creating a follow-up ticket if there are further improvements. I think that commenting without approval should be sufficient for style concerns, etc. that will not cause harm upon push.
What kind of feedback are they talking about if there is no improvement to the performance? Is it stylistic/idiomatic things?
Leather_Opposite_452@reddit (OP)
It’s like 90% things like some I mentioned below:
That might be, using a different array method than the one you used because it’s more readable to them
It might be lightbulb ah this is kind of similar to another function or component somewhere else, can you investigate making a shared component to handle both of these things
Can you use a record instead of a ternary so that it is more extensible in the future.
Variable names
Truly scrutinizing everything for if there is any possible way it could be “better”
thekwoka@reddit
Those all seem pretty dang reasonable.
Leather_Opposite_452@reddit (OP)
It’s driving me a little insane because I’m literally spending days at a time just addressing feedback items and then feedback from the feedback
SpookyLoop@reddit
My current manager is like this. I eventually lost my shit, reached out to a director that worked in an entirely different department, and had a one-on-one with the CEO (pretty small company, 5 SWE and only ~80 employees total).
I ultimately still have to deal with it, but it was nice getting someone to basically say "yea we know, but we can't fix the problem" and giving me a pay bump. Slowly looking for another job.
RelevantJackWhite@reddit
How do they react if you push back on these things? Are you able to point to a style guide to defend your decisions?
nicolas_06@reddit
Follow up PR are difficult or impossible to enforce so that depend a lot of how the individual that did the code will handle it. There some people you know they will do it they people you know they will never do it.
thekwoka@reddit
What about maintainability?
And how many of these things can be handled by robots? You should have linters and formatters enforcing those kinds of rules whenever possible.
b1-88er@reddit
Sounds like your colleague is a human linter. Annoying and inaccurate.
pinkwar@reddit
It makes sense to nitpick on junior devs.
If the dev is a senior some things are just better let alone.
nf_x@reddit
Code is read thousands times more than it’s written. That’s the guideline for any review 🤷🏻♂️
bouncycastletech@reddit
Every like of code should be scrutinized.
However, there’s a difference between “this is a wrong/bad/nonstandard way to do this and this is a personal preference”.
I watch some people make code comments about how they’d do things, not about what’s correct.
Leather_Opposite_452@reddit (OP)
When I say “scrutinized” I mean that this reviewer is looking at the line and thinking is there ANY way this could be better.
That might be, using a different array method than the one you used because it’s more readable to them
It might be lightbulb ah this is kind of similar to another function or component somewhere else, can you investigate making a shared component to handle both of these things
Can you use a record instead of a ternary so that it is more extensible in the future.
Temporary_Emu_5918@reddit
Reuse is important. I had a colleague level higher than me that refused to use the splashscreen component and made his own. It looks the same, but now if we have to change the splashscreens, then we have to go through each page and check they're using the same/different splashscreens, change those ones or refactor ourselves. So it's not that he's saving work, he's just passing it onto us to fix. Sadly he's a team lead, so can't do much about that.
Leather_Opposite_452@reddit (OP)
When I talk about re-use I don’t mean literally recreating the same thing.
It’s more like “oh I noticed you created this new function, can you implement it across all these areas of the codebase since it would be useful” such changes might require architectural changes to a few different areas and changes in integration tests etc
sammymammy2@reddit
"Let's make this into an unassigned follow up ticket, feel free to take it if you have some spare cycles"
germansnowman@reddit
This. Most actionable items that are beyond the immediate scope of the original task should become new tasks.
Temporary_Emu_5918@reddit
Oh yeah I'd just hit them with "outta scope" but probs gets exhausting if repeated
besseddrest@reddit
ugh i'm already so annoyed
This is prob just an arbitrary example but to this i say - "so you can read the method i used and follow what its doing, but you want me to use this other array method, because you think it reads better?"
If they can't show me what they're referring to and I need to get this code merged, make it a ticket and i'll look into it later
I prob don't have enough context here but, this is kinda along the lines of something I can make for a task in the next sprint because it wasn't part of the original scope - I'm more inclined if 'the future' means that this change request is going to set us up better for something related in an upcoming sprint. Not some vague future.
rayfrankenstein@reddit
People like your colleague are the reason I reason I feel that the practice of code review should be banned from the industry.
besseddrest@reddit
wut
Code review is 100% absolutely necessary.
Their colleague is not even a good enough reason to argue against code reviews - that type of coworker exists at every single company. In fact the suggestions aren't terrible but they've obviously cause enough frustration for OP to feel the need to post
My viewpoints above don't really match OPs context - the code review process I found most effective was on a team where the engineers generally had the same skillset and could work on any part of our repo interchangeably.
The pace of our development was fast, and so come review time, making suggestions ad hoc to like, rework some 'possibly' shared component that they can't quite remember, that I now have to dig for, is just a bit of a distraction - when we have to move our tasks through to completion rather quickly
InquisitiveDev645@reddit
The "make it right" part refers to refactoring your code to follow best practices, e.g. making it simple, readable, obvious, easy to change, easy to test (+ well tested), and well organised.
These things are extremely important for the long-term health of your codebase. Code that doesn't meet these criteria will come back to bite you later on, e.g. it becomes harder to read and understand (and takes you more time and energy to do so every time you look at it), harder to test, and harder to change or extend.
Think of it as a force multiplier for you and your team: by making your code nicer now, you're going to reap the rewards from that effort every single time you need to read or work on that piece of code in future, which can end up being hundreds of times or more.
InquisitiveDev645@reddit
As another comment mentioned, a good linter setup should take care of things like this automatically. You're both right - it's important to keep the code as readable as possible, which needs everyone in your team to write things in a shared consistent way, but it can be very draining to have to bring this up in code review comments and can cause you a lot of frustration. Having a linter check for this will take the emotion out of it and make you all happier.
zapman449@reddit
Really simple rubric: are you willing to be oncall for this code in production?
Leather_Opposite_452@reddit (OP)
I would say absolutely yes since the large large majority of comments have no impact on performance or outcome of the code
0xfreef00d@reddit
Sounds like you have your answer. Code is just a means to an end.
JWolf1672@reddit
There is over scrutinizing, but things like improved readability is important and having someone check if another function or component can be re-used is also important for the future.
I've had to respond to incidents where things took 3 times as long as they should have to diagnose and triage because the person who wrote the code didn't put any care into making a complicated operation readable. Likewise not trying to reuse existing functions is how you get 4 different ways to parse a datetime, one of which will get missed when something inevitably changes, causing an issue.
I'll agree that I wouldn't usually hold up a merge over something like that you could have used a ternary here instead of something else though.
jdx6511@reddit
If I can't quickly come up with a succinct explanation of how my way is better, I move on.
Uncontrollably_Happy@reddit
This. I like to be nit picky, but I also like to differentiate between what needs to get fixed versus what would make the code easier to maintain. When all the major issues are fixed, I’ll approve the MR but leave the nit picky comments up if they’d like to correct them.
Simea@reddit
My personal standard: “is this going to fuck something up? If not, ship it.”
horserino@reddit
Every line should be scrutinized until I can trust the author to have the discipline to do the right thing for the codebase's future and the maintaining team's future.
PR code reviews can be a moment to communicate team's standards to the PR's author. If every line is getting scrutinized and commented on it could simply be that your reviewer does not trust the quality of your job.
Of course it depends on the context, whether it's core code for the product or throwaway stuff (although do not underestimate the amount of "PoC" code that finds itself in production), whether you'll be working on the same thing right away, whether it is a hot path or not, the current state of the rest of the codebase, etc.
So all in all, I'm from the team that code reviews should be pretty strict. I've worked with old and new codebases and strongly feel that the burden of sloppy code on maintenance very quickly piles up and the small annoyances of strict code reviews today pay for themselves tenfold in the future. And the best way, IMHO, to deal with that feeling of annoyance is to project your empathy on future maintainers of your code (yourself or someone else).
ben_bliksem@reddit
There's a point where you have to ask yourself if what you are commenting on is really better or just personal preference/opinionated.
Thats the point where you read the room and possibly stop.
waterkip@reddit
It depends. Timtowtdi as we say in perl. Yeah, you can optimize the hell out of things too early, or you yagni it, or you build castles where only a shed is needed.
Does it work? Is it safe enough to try? It also depends on the size of the PR/code change. The intent of the change etc.
popovitsj@reddit
Honestly it sounds like there are a lot of issues with your code and you don't handle feedback well. No, it's not just about "it works".
Especially if you keep getting the same kind of comments, that would be a bad sign. If you get a comment about something and you agree with the reviewer and fix it, your next PR should not make the same mistake. If that's the case, how is it possible that you keep getting so many comments on your PR's?
Leather_Opposite_452@reddit (OP)
What gives you that impression? Obviously you have 0 reason to take my word for it, but I am someone that always leans toward self critique first before criticizing others. I question daily whether what you say is actually the truth.
However, my code is not just thrown together and “works”. I review throughly and strongly consider best practices, readability, shared functionality, duplication etc.
I focused on questioning about the rationale “every line should be scrutinized to see whether it can possibly be better” to avoid bias. That is not my read on the situation, that is a verbatim quote. It seems like most people disagree with that rationale.
zica-do-reddit@reddit
For me what's important is : 1. Security: any risk of leaking information? Any exploits?
Functionality: does it work? Any uncovered edge cases? Potential bugs?
Performance: will it be fast enough? Can it be improved? Any unnecessary steps that can be removed?
Structure/clarity: is a method too large? Is it hard to read? Does it lack comments here and there? Does it have too many classes or interfaces? Too few?
I almost never nitpick, it's not a reason for holding a PR. Remember: you are reviewing the person's work, not doing the work for them.
_GoldenRule@reddit
I mostly care about readability (and tests). I'm of the school that you will catch bugs when your code goes to prod and if your PR causes a bug you are the one that needs to monitor and fix it.
If you have extremely long PR cycles with a large back and forth you're tying up 2 engineers for a dubious benefit. I think that occasionally this is warranted (think a really crucial feature/large blast radius changes) but for most changes I think it's too much effort and time.
aFqqw4GbkHs@reddit
Sounds like too much time investment for both of you and probably not much ROI. Obviously, depends on your business context on how much scrutiny is needed, though.
FlipperBumperKickout@reddit
I give feedback on everything I think can make readability better.
Also sometimes when I think they do something which makes it unnecessarily hard to review which really should be split up in multiple PR's or commits.
rco8786@reddit
As you probably suspect, this sounds a lot like overkill.
A good linter should eliminate almost all stylistic review comments. And otherwise the body of the code gets reviewed for correctness (does it do the thing it's purporting to do), performance (are there any glaring performance issues relative to the scale we expect this code to run at), and failure (does it handle failures gracefully and allow for debugging).
nicolas_06@reddit
Performance for most use case is overkill. I did work quite a lot on performance and 99% of the changes people do to improve performance do not improve performance, they even sometime make it worse.
If performance matter, you need to do profiling, find the 1-10% of the code where 90-99% of the time is spend and focus on that and you also need to have automatic performance test that would warn you of a significant degradation of performance.
All that has quite a big cost and should be paid by the project with the clear metrics of what the software should be capable on what software (we should be able to substain 100tps with this scenario) and not just have perf for perf.
Code review alone will not do much here.
edgmnt_net@reddit
There's a difference between dumb/premature/costly optimization and doing the sensible thing, though. Code review will absolutely catch unexplained and wild deviations from the latter. It could boil to nothing more than picking decent combinations of functions or data structures already present in the language/libraries you're using.
For example, if you have easy access to maps you probably shouldn't be iterating through lists to keep track of various associations. Although in that case it can be argued that typing and style make it an obvious choice before performance concerns apply, but it needn't always be the case.
nicolas_06@reddit
Using map vs list would depend a lot on the size of the map and the type of list (linked list ? Array list ?) and map (hash map, tree map ?) and also of the memory available. it depend a lot on the complexity of the key, how the data will be statistically and what algorithms you'll want to apply.
If you have association with a limited numbers of possible elements, pure array (and list tend to be backed by arrays) might be faster and more efficient than maps because you can use the index directly.
Using specialized data structures can often improve performance drastically (a very specific type of map instead to the standard one). I have already seen a perf gain of 3-4X.
All that is very subtle and often counter intuitive. For example, the theoretically faster data structure in term of O notation may very well consume more RAM and lead to more fragment RAM and hurt the CPU cache and lead to worse results.
On top, 95-99% of the code isn't really sensitive to perf and it won't matter. If that code isn't run millions/billions of times the perf is likely irrelevant. It may also be that other factors are much bigger bottleneck like the network or the database.
I insist, if you don't have a performance objective like the system must be able to sustain this level of perf for this transaction with this hardware, honestly you waste your time. On once you have something like that, do benchmark and profile it and optimize the 1-10% of the code that is responsible for 90-99% of the cost and don't care if you use a map or list to store 3 elements at the init of the program.
edgmnt_net@reddit
I think I need to add a few things to this:
You seem to treat it as a significant recurring cost, while if it's merely best practices it's more of a learning cost along with a tiny recurring cost, if you can count it at all. You get used to doing some things a certain way, much like you get used to handling errors or using types properly. This is basically more along the lines of best practices and common sense rather than going entirely out of your way to do questionable stuff that may or may not improve performance.
You need people to grow. They won't suddenly start working on highly-optimized code without proper practice. They need a solid background by the time they get into optimizing the more important stuff. They need to be familiar with the language and the ecosystem.
Small pieces of tech debt do accumulate. We know they do when we look at other things that are not performance-motivated (e.g. maintainability, lack of static safety etc.). Then it may take more time to fix up the mess or you submit to never get back the lost stuff. Which could be fine or not, it really depends on whether the overall expectations are realistic.
If performance isn't important, then just applying common sense optimal patterns isn't going to hurt much either, even if it turns out to be counterproductive in a significant number of cases (maybe unless it's a majority of cases). Benchmarking and profiling is also an effort expense that you have to account for, that's not free either. So there isn't much justification for going out of your way to avoid common sense stuff, it could even be seen as premature deoptimization and going against the grain.
Some of these "optimizations" go hand in hand with other concerns like making illegal states unrepresentable or reasoning about the problem at hand, e.g. knowing you have a map eliminates impossible key duplication even if it doesn't improve performance.
That being said, I do agree to a large extent, especially regarding core algorithmic gains on datasets known to be large, guiding optimizations by profiling and network speed being a much more significant factor. So, yeah, don't waste time on premature optimization, but you don't have to pick the most stupid solution either. Reviews will help people grow and keep basic things in check, even if they don't yield a grand design for the more important stuff.
nicolas_06@reddit
I agree one should not pick design that are clearly bad if performance is a concern. But such things are overall part of the global design and should be known in advance long before the code review. I would even go as far as to do a study and prototype and benchmark to double check the actual architecture meet the requirements.
But once you are in a code review, all that should be already documented and clear.
That's why I mostly disagree for point 4. You don't do random optimization in the void as on top they usually make the code worse (less maintainable and readable) and half of them are obsolete.
I also disgree with 5. 5 come from the necessity to be corect, not performance. You might decide to change the key or to have a list/set/bag whatever as value like when you do a "group by" algorithm.
To do 1,2,3 if you want people to learn doing code review with best practice will do 1% of the job and it will be very hard for them to arbitrate when to do it or not. You absolutely need a benchmark in place and have available profiling tooling.
Maybe the newbie will write 100 PRs and none of them will be a problem and they will be able to mostly ignore such concerns. Maybe under the supervision they will be asked to do a new benchmark for a new key functionality/flow. Maybe at some point they will be tasked to improve perf here or there or will have to correct a code that made the software too slow. They will learn without issue only when necessary and with the help of their mentor only when necessary and progressively because all would be in place and they would be introduced to that progressively as issue happen or as they have to work on this or that part of the software.
It is 100X easier to learn about performance if people explain you the best practices to have actual clear objectives, to have actual way to measure and assert if the product meet or not the expectations and then if there are tools that allow to investigate where the resources are consumed to get an intuition of where to optimize the code.
If actually at the PR level the performance test failed, you already know that the code of the PR is the likely culprit.
That's much better than to do a few obscure change at PR level that should protect you without understanding them, load to prod, have a big issue, panic and have to come up with everything instantly at 3am while being on call.
rco8786@reddit
I'm talking more like low hanging stuff. N+1 queries, unnecessary network calls, etc. Not like benchmarking every line.
EkoChamberKryptonite@reddit
Hear, hear.
tehfrod@reddit
While OP's colleague is probably over the line there is more to consider than that.
Maintainability? Accurate naming? Not using known error-prone APIs? Testability? Simplicity?
Careful_Ad_9077@reddit
It called me attention that op did not mention those, just performance and it works.
rayeranhi@reddit
I'm in the same boat.
Wish we had ship show ask https://martinfowler.com/articles/ship-show-ask.html
Break PRs into baby chunks that take like 10 minutes to view at a time.
Separate style/refactor PRs and logic changes might help?
elusiveoso@reddit
My general rule is not to let perfect be the enemy of good. There is code that I will approve if it is good and it functions even if I would have approached it a little differently.
My other rule is like sparring in a combat sport. Hit people as hard as you are willing to be hit.
birdparty44@reddit
I think the level of scrutiny has to be weighed againat the impact of the component on the codebase, and how many others may have to interact with that code.
the nitpicks are really for engineers who have lost sight of how business works and how they get paid. It’s expensive to be going back and forth over someone’s opinion on something of no consequence.
Establish a bunch of guidelines about code review in your project documentation then invoke/refer to them and avoid needless back and forths.
mxdx-@reddit
Depending on the level of your team. Mature teams makes it almost useless to review code, while younger, inexperienced members may benefit more from a more thorough review.
The key is kindness, if it takes more than 1 comment to make yourself understood, go grab a coffee with that person or give them a call.
I think that to scrutinize every line of code in a seek and destroy fashion ultimately leads to poor team's morale and evitable stress.
Comprehensive-Pea812@reddit
Depends on the organization budget.
most people want quality but refuse strict reviews
chmod777@reddit
Thumbs up, close issue.
fancy_panter@reddit
You want me to review how many LoC on top of my already extremely demanding job? Lol have a green check.
DaRKoN_@reddit
LGTM, shippit!
Codex_Dev@reddit
YOLO merge
ziksy9@reddit
It's gonna be deprecated in a few years anyway. You aren't writing code for NASA. If it's not a security issue or glaringly bad, Ship it.
daredeviloper@reddit
From my experience it depends on who I’m reviewing. People get reputations for introducing bugs. So I scrutinize their PR harsher. Juniors get more advice and questions. People I trust I just do a quick once over.
MocknozzieRiver@reddit
Personally, I like it. I want to know people's opinions so I can log it away in my head to consider their thoughts later.
Like if someone's like "I find it easier to understand when x," that's valuable information because we are both in this team looking at this code all day. We're faster if I can tailor it to everyone's preferences as possible.
lokaaarrr@reddit
All syntax should be an auto-formatter
All the basics should be a style guide
Everything else is what matters, and you should come to agreement
bigorangemachine@reddit
I generally say if they have some arbitrary feedback (like javascript using async/await vs then/catch) there should be a linting rule for it
If formatting feedback can't be automated I don't want to hear it.
I often leave optional feedback just saying "you can do it this way... might be better to scale later" more noting we might need to change something later.
I'll leave it to the coder whether they want to think that far ahead or not. Not a lot of people do.
zhivago@reddit
There should be various domains of responsibility.
A team review, a readability review, an ownership review.
Of course, one reviewer can wear many hats.
Any-Ring6621@reddit
2025, linters and formatters should be in place, and an AI reviewer like copilot, reptile, or Claude should be the first reviewer.
After all that’s done, the only thing left to talk about are basically business logic/contextual edge cases
JaneGoodallVS@reddit
I'm pretty lenient. Does it have maintainability issues? Observability issues? Bugs? Performance issues?
If you're "strict", people have to context shift to address the feedback and they'll start putting up minimal possible change PR's instead of paying down tech debt. It lowers morale so they'll start mailing it in. Seen it many times.
Forward-Subject-6437@reddit
Tangential to the question, but CodeRabbit is amazing. Use it locally and in CI/CD and you can eliminate a lot of that before it begins, saving manual code review for that which requires a human perspective.
pawbs@reddit
Maybe a hot take, but the risk of too many comments is that the PR becomes stale and becomes a gruelling exercise to resolve conflicts.
Keeping this in mind, for me, the first review can have as many comments as the reviewer deems necessary to keep the code base up to standards. But after a bit of back and forth, maybe it’s worth it to split off the work into a separate ticket and slap some TODOs in it
You also shouldn’t have this problem if you keep your PRs small. Commit and merge often
Shnibu@reddit
Leave things better than when you found them. Always be improving code standards but don’t slow down the path to production. Sometimes this can be better/automated testing or linting and other automated commit hooks. If documentation requirements are slowing down production then that is a problem. I see code review as being a good growth opportunity and should highlight areas for improvement but there is a difference between “next time you could do this” or “maybe later we should refactor it like this” and saying “you need to change this before we can merge”.
Leather_Opposite_452@reddit (OP)
The problem I find is that the reviewer can sort of start from 0 when looking at my code.
It may be the case that I have already improved a lot of areas of the code that I have I have touched, but I have to draw the line somewhere. However, it feels like the reviewer abandons that line I have drawn. There is always additional ways in which things could be shared and improved further like a kind of chain reaction of improvements that delve deeper and deeper into the code base.
SynthRogue@reddit
Depends on how much of a narcissist and stuckup asshole the reviewer is.
nicolas_06@reddit
This depends a lot of the context for me. If something need to be delivered fast for whatever reason like the project is late or whatever, you want a minimal code review. The code should be decent but having the best code in that context is really stupid. In such case the essential is that the code is tested, there are no big bugs or risks and the code is somewhat readable and maintainable and that's about it.
Also for a beginner or somebody that would really struggle make it perfect, you want to focus on making them learn the essential first. When the core things are mastered and they do it by themselves you can be more strict.
And that's my point. A good code review that will show how it can be improved, with link to why it's better that way and showing to people how they can try to search for alternative and look for the best code is extremely valuable as a learning tool. After 6 months to 1 year doing that, they should be able to do it by themselves and increase their code quality right away.
Spending more time improving yourself is making you faster and more productive in the long run. Somebody that will spend an extra half a day a week improving himself will soon (in 6 months to 1 year) be faster than if they didn't do it.
This is not code review alone but combined with pair programming and mentoring showing people how to improve and learn by themselves.
As much as possible things should be friendly and showing what is possible and not judgemental.
If you are reviewing senior level dev but the quality is low, you can't be too harsh or it will be seen as lack of respect, so focus on the essential and try slowly to show some extra improvement here and there to help them progress if they are receptive.
If they are not receptive, as senior, honestly then that's their problem. They will have more difficulties to evolve technically, but ultimately that's their choice to not improve themselves.
Leather_Opposite_452@reddit (OP)
There is 0 consideration from my colleague for whether something is late or needs to be delivered fast - the feedback must be addressed period even when it has 0 impact on performance or outcome of code.
This is what truly creates the stress because management are like wtf is going on, and you have to report that you’ve spent days addressing minor improvements with no impact
daraeje7@reddit
That’s pretty much all I do.
pl487@reddit
The default should be not to change things at the PR stage unless they are broken or do not meet the requirements. There are exceptions when it's really off the pattern like installing a whole new library to do a thing when we already have one. But a variable naming comment should be something that you are free to change or ignore as you please.
Nofanta@reddit
Opposite of this. Hire good people and trust them, accept different styles and ideas. Code review is just to spot huge problems.
Leather_Opposite_452@reddit (OP)
I think part of the problem is that my impression I get from my colleague is that there is kind of an objective way to write every line of code that they cannot understand why you did not do the first time
general_00@reddit
The strictness of code review should depend on the criticality and impact radius of the change being reviewed.
Most people intuitively understand that a commit to Linux kernel should be reviewed extremely strictly, while changing a button on the company's internal website can get an "LGTM".
mechkbfan@reddit
In general, I agree these discussions around style, conventions add zero value after the first one or two. I dont care if its tab or spaces, I'll adapt to either one.
So as a team, come to a consensus around code styling, preferences, etc.
Automate as much as you can (.editorconfig, code linters, etc.), then document the rest in shared Wiki.
After that, you'll no doubt discover some you forgot to talk about (e.g. Sentence structure for logging, naming methods, etc.), Repeat the process, and add those conventions to Wiki, or at least your own notes to refer back to.
By then you should just be getting feedback around defects, performance issues and the odd subjective issue such as readability of code.
jeremyckahn@reddit
I'm pretty lax with them. I mostly just skim for obvious functional/performance problems. AI generally does a better job with code reviews than me or any human I've worked with. If an issue is missed, we just fix it later when we find it.
This approach has worked quite well and keeps things moving, which is what really matters.
samsounder@reddit
That’s terrible.
Code review is to make sure you can fix it if the author isn’t available and to make sure there aren’t huge architectural mistakes.
Scrutinizing every line is a recipe for a very slow and argumentative work environment.
I almost always favor code that’s easy to read over an optimized line. Disagree if you want, but that’s a meeting on code style, not code review.
Leather_Opposite_452@reddit (OP)
I’m finding that code has to be both as optimized and as readable as possible. Every line is being critiqued on both of those bases. That also includes the responses to the feedback, so feedback is given on top of feedback
samsounder@reddit
Yeah….
That takes forever.
Are you spending more time building things or code reviewing?
It also depends on the maturity of your project. The more mature the project, the more you need to look for unintended consequences
Leather_Opposite_452@reddit (OP)
I’m spending equal time probably building and responding to feedback.
It’s also causing me to hesitate on how I write my code because each line I am second guessing how this will be critiqued in this way. It’s horrible
samsounder@reddit
Good trick in situations like this is to purposely leave a think of code in the wrong place!
“Oh, I should have that in this service over here? Great idea!!!”
Approced
serial_crusher@reddit
Automate formatting stuff. If the linter fails, the PR doesn’t get merged. No need for discussion. Somebody doesn’t like the way the linter is configured, they can make a PR to change the linter config, and discuss the merits of their change there when time permits.
Let people ignore nitpicks. This variable name isn’t the best? Meh, I only change it if I’m making other more substantive changes.
Make it clear when something is a blocker.
If somebody is treating nitpicks as blockers, have some conversations with them / the team / their manager to get consensus in place.
Leather_Opposite_452@reddit (OP)
We have a linter and we also have extensive integration tests which block merging - so these are not an issue.
It is more feedback on overarchingly optimally integrating each new bit of code into the code base, structure of unit / integration tests, optimising shared functions and components - which is what makes it difficult to pre-empt how the code should be written.
gimmeslack12@reddit
Are we getting things done?
Or am I being pedantic?
These are the questions I ask myself when reviewing code.
SpookyLoop@reddit
I'm of the opinion that the absolute best mindset to have for code reviews should be that they're "learning opportunities to help devs grow", and that it all boils down to: catching mistakes, objective improvements, opinionated suggestions, and asking questions.
As for strictness, anything that's a "caught mistake" or an "objective improvement" should be pretty "strict", but if everyone's ego is in check, those shouldn't need much discussion or a need to be "strict".
"Opinionated suggestion" should always go back to some sort of "bigger picture solution". If the "opinionated suggestion" can't go into some sort of documentation like a style guide, or into some sort of configuration for something like a linter, chances are, it's a bad suggestion to begin with.
"Asking questions" is usually very harmless, but some people do go overboard with it. Questions should never be a blocker, and should always be optional. If the reviewer has an issue with something, they should clearly state the issue.
Beyond that, code reviews are a waste of everyone's time. I have zero respect for devs that take pride in things like "how many bugs they can catch in a code review". That sort of mindset provides negative value to teams.
Kolt56@reddit
Your peer is the essence of bureaucracy. Assuming your post isn’t biased.
How many lines are your code reviews. And who is your audience?
If you are a SR reviewing a SR at FAANG;… you provide deeply technical nuances, maybe a protocol adjustment, you will fight to the death.
Sr/Mid FAANG reviewing a mid/jr/intern;
More frequent, smaller size is key.
Any JR or intern or lost in space mid who isn’t required to do this, will wait until it’s an EM/product problem, since there are optics, you get to eat the bowl of shit they just flushed you as a MVP(ohh you required TS, here I renamed them all from js)
What I’m saying is this is not pragmatic, nor is it scalable.. this is bureaucracy. Unless you are programming a pacemaker or need to make sure the function that times the movement of the control rods is not going to cause a ‘people-ops’ escalation, this is indicative of a developer on their way to pip land.
insta@reddit
idk OP, how many times do you have to be told in your PRs to do it a certain way and you're not doing it that way?
give examples on the kind of feedback received and the code that generated it, please.
Leather_Opposite_452@reddit (OP)
We have a system of noting when a specific feedback item has been previously mentioned so this rarely occurs.
I have mentioned examples in a few different comments e.g https://www.reddit.com/r/ExperiencedDevs/s/jySyw9Hawx
mr_brobot__@reddit
As a lead/one of the more senior team members I do pore over most lines of code closely, but I
It’s a balance, but I do agree with paying very close attention. I catch a lot of real issues in PR review.
Leather_Opposite_452@reddit (OP)
I think there is maybe a misinterpretation of what I mean by “scrutinize every line”
I agree every line should be looked at and considered.
However, what I mean by scrutinize every line is that each line is looked at and if there is any possible way that it could be improved no matter how minor - that is commented on and requested as a change
mr_brobot__@reddit
You need to find some alignment with your team mates along the lines of the bullet points that I outlined.
kenflingnor@reddit
This sounds like a waste of time. Yeah, there’s probably always room for improvement, but this ultimately ends up in PRs taking forever to merge because Johnny is willing to die on some hill about X piece of code being changed, despite it being a nitpick.
The two things that have helped teams I’ve been on deliver while also maintaining a high-quality code base: conventional comments, and using a linter/formatter with defined rules that run on all PRs.
andymaclean19@reddit
It depends on what you're doing. If you're making a space shuttle flight control system or a machine which does heart surgery then that's definitely true. Most of the time not so much.
Most software doesn't need this level of scrutiny most of the time. As a general rule you want code to be flexible and to change and adapt over time. The more effort you put into tuning, optimising and perfecting it the more is lost whenever you start changing everything around and having to do that all over again. The effort:reward ratio just doesn't justify it.
Some software simply doesn't live long enough to do this at all. A lot of things I've worked on had a core which was worth treating like this because you would tend to make it once and then rely on it for many years with little modification. Then most of the stuff around that core is a lot less critical and features, robustness and development speed are a lot better than getting everything 100% correct.
Generally if you let teams be too perfectionist in reviews my experience is people will eventually disagree on things in cases where both answers are good enough and spend a long time in a state of 'design paralysis' trying to figure out which 'good enough' idea is best.
Just divide the review into 'must fix' type of comments and 'have you thought about doing it this way' type things that are optional.
technowomblethegreat@reddit
Human review is almost always poor. PRs are always too big. People don’t really read big PRs.
People never budget any time for it, struggle to get them to do it outside whatever their priorities are. Most people don’t enjoy it.
The SWEs that are complete pricks, and they are plentiful, use it as an opportunity to be complete pricks. Nit on things they shouldn’t be and rub people the wrong way.
Automate and unit test as much as possible.
If there are no obvious bugs and nothing horrendously wrong with the code, that’s good enough. If you’re writing software where people might die if it goes wrong, may need a higher standard.
Leather_Opposite_452@reddit (OP)
My colleague absolutely does read every single line of every PR
technowomblethegreat@reddit
Engineering manager should be having a word and coaching him not to rub people the wrong way.
This is nitpicking.
If it isn’t substantially improving outcomes, it serves no purpose.
lambdalord26@reddit
Stylistic and best practice items should be enforced by automation in the pipeline. Beyond that, I tend to be the strictest when I first review a PR. After that, if I notice something on a follow up review, I tend to be more relaxed because I don't want to keep bugging the person.
MangoTamer@reddit
That's too much. Tell him to get over his own ego.
drnullpointer@reddit
Code review is... already too late.
At the code review there is a huge incentive to just pass the code and not make a fuss. The person who wrote the code is probably facing some kind of deadline and wants to get it done ASAP. The reviewer may fear confronting the author for fear of retaliation (when the roles are switched).
The types of things that are fixed on code reviews are huge obvious issues or superficial stuff (just so that you do seem to be paying attention). Everything in between, everything that causes the deterioration of the codebase, does not get fixed.
What I prefer instead of code review is pair programming. Two people working together on an issue, understanding the implementation deeply and discussing best way to get the job done right from the start.
Leather_Opposite_452@reddit (OP)
I agree with the idea that each line should be scrutinized in the sense that it should be looked at and critiqued on whether it is performant, fit for purpose and readable
The problem in my view is that the way that this is being practiced is that it that it’s looking at each line and thinking of any kind of way it could be improved on any level. Down to nits or just truly any improvement you could think of.
Beneficial_Map6129@reddit
I just stamp things now after a cursory glance. Way too much damn work these days
neosituation_unknown@reddit
That sounds over the top.
There is value in a robust review, obviously, but this sounds like pedantry
inputwtf@reddit
Are they providing suggestions that are actionable? Like do they provide a proposed code change? GitHub for example has the
suggestionmarkdown macro that can be used to propose changes.0dev0100@reddit
Use a linter.
Then a code style needs to be decided and enforced using the linter where possible.
A "good solution" needs to be decided on.
Then the implementation needs to be "good enough".
National_Count_4916@reddit
Risk > Productivity > Pedanticism
There can always be another PR. Blocking a PR is an impediment and not to be done lightly as it also triggers ego fights, even in no-ego shops
canihelpyoubreakthat@reddit
There's a lot of nuance to it. Code review can be a great training opportunity for a new hire, or a great chance to share context and ideas.
But there are diminishing returns. Your coworker is an extremist and wrong.
Wooden-Glove-2384@reddit
we've started using AI for code reviews and that's turning up stuff a damn sight better than the "you used too many blank lines" or "get rid of all those comments" and other such silliness I've heard
VelvetBlackmoon@reddit
What is committed is a statement of what is acceptable for your organization. Don't let it fall under that.
sekerk@reddit
Sounds like they’re overworking — some things are best to take into brief consideration and move on from if it is really not impacting the target outcome.
Is it a matter of making things more reusable or more modular? Then yeah work that into a refactor or take it for another discussion to group in with some other refactors imo
donniedarko5555@reddit
Some solutions
If this is eating into productive work time without much gain, reach out to your engineering manager to resolve this issue. Include these recommendations and any others you come up with.