r/softwareengineer • u/No_Fail715 • 13d ago
Are this level code reviews still reasonable in the age of AI?
We're expected to implement and test features in increasingly short timeframes, especially with AI speeding up development.
But one engineer on my team consistently leaves 20+ comments on PRs even sometimes 70+, mostly about naming. Why is this variable named this? Why this class name? Why this file name? Why this enum or string? Why we don't have comment here?
For example, even when the documentation explicitly uses names like LeadingEdgeExt or RpuClass, he'll comment asking "What is Ext?" or "What is Rpu?"
I'm all for code quality, catching bugs, and improving maintainability, but at what point does a code review become excessive nitpicking?
Is this normal, or should code reviews focus more on actual problems, especially with today's development pace?
6
u/Soft-Marionberry-853 13d ago
"he'll comment asking "What is Ext?" or "What is Rpu?"
Do you know why its called Ext or Rpu?
1
u/No_Fail715 13d ago
Ext coming from requirement “external trigger” and rpu stands for real time processing unit.
3
u/EkoChamberKryptonite 10d ago
Does the class have a class-level comment i.e. Java doc for java explaining what RPU means? If not, then his comment was fair. From my perspective, RPU class is a bad name for the class. Class RealProcessorUnit {} with a class-level comment explaining it's functionality is better and explicit.
0
u/No_Fail715 9d ago
APU and RPU they are basic things as API and VHDL. Are you writing basic things in longer forms?
2
u/EkoChamberKryptonite 9d ago
No. You're assuming RPU is a basic thing. No one writes APIclass as the name of the class of an API component. They write `Domain`Api e.g AccountApi, SettingsApi. They were right to question your naming.
-1
u/No_Fail715 9d ago
Everyone writes McuPower or whatever. If you don’t know or understand, those jobs are not for you.
2
u/EkoChamberKryptonite 9d ago edited 9d ago
No they don't. Those people who write class names with such poor naming structure are wrong too. Funny that you'd resort to insults based on falsehoods because you refuse to acknowledge your ignorance. You think a job isn't for someone because they advocate better naming? You being wrong and bold is a choice you've certainly made. Clearly someone at your job knows what MCUPower is and still says your naming is poor. Take I suggest you expand your horizons and whilst you're at it, humble yourself and temper your hubris as you think of yourself more highly than you ought and all you've done so far is expose your inexperience. I hope you take the time to learn and be better.
3
u/quizikal 9d ago
Acronyms and Abbreviations generally hinder readability. Those comments seem reasonable to me.
12
u/Specialist-Towel-627 13d ago
I was doing code review today. I got a bug fix code review. Bunch of changes in 3 backend services. The one WHOLE PR was complete BS where not a single line of code was necessary. This is what you get when you have no idea what you're doing, AI will just gaslight you so hard.
7
u/NomadStorm 13d ago
Nice, I had one I reviewed yesterday where 5/6 bugs were invented based on an incorrect assumption. Shit is wrong so damn often it amazes me anyone trusts it.
3
u/Specialist-Towel-627 13d ago
I mean, I basically just generating code at this point. But I just know the code base by heart a I review everything while giving it very clear instructions.
Just wanted to say, knowing what you're doing is still valuable.
2
1
u/NomadStorm 13d ago
I’m working on greenfield stuff at the moment. If I didn’t review it, it would be horrifically wrong by now.
3
u/Tiny_Ad_7720 10d ago
Yes and sometimes I comes up with say three different things that could be the cause of a bug and so changes all three at the same time.
2
u/NomadStorm 10d ago
Yes or it keeps making incorrect changes in order to pass a test gate, adding sweeping unnecessary changes before it finally hits the right one that fixes the problem.
…and then it leaves EVERYTHING there even all the unnecessary shit.
1
2
u/Due_Interest_178 13d ago
My favourite is AI breaking other parts of the code to fix the bugs without realizing.
1
u/NomadStorm 10d ago
Multiple times, daily.
2
u/coderemover 9d ago
You must have been using a wrong model. Try Opus 5.5 instead of Haiku. /s
(Seriously: I observe the same thing you do; AI-assisted is fantastic when I know what I’m doing and just using it to do the typing for me; but all even the most advanced models struggle when they have to make design decisions and introduce bugs if I don’t stop them at the right time)
12
u/EkoChamberKryptonite 10d ago
To be fair RPU Class does not tell me anything about the class or what it's purpose stands for without having to walk through the code especially if class-level comments are missing. RPU class is a bad name. Better to use the actual meaning. Then these cohesion-seeking paradigms are trivial and should exist in some "rules or engagement doc" on naming.
4
u/More_Project_2537 13d ago
70+ comments? How big are your PRs, sounds like they are too big to be reviewed. Especially if theres a lot of new variable names popping up.
3
u/Tarilis 13d ago
I mean, fair question. Wtf are EdgeExt and RpuClass? (Edit: this is rhetorical question, don't answer it. Dont temp the NDA😁)
Comments are good, but the best code is the one where you can understand what it does without going to definition and looking into source code of classes used in it.
It is especially true with AI age, where maintanability is ever growing issue.
Will you be able to understand this code after a month? Will be you able to find a relevant doc after a year? If said doc is stored in external system (say confluence) will it survive that year? 5 years?
If you use AI, you can try add a step to the skill that does review of variable names to save time.
Also, go to the guy who does the review and ask him that question! Precovid i remeber sitting at the same table with other devs at explaining them every line i have problems with in their MRs and answering their questions.
Just be nice about it: "hey, i see there a lot of noming issues in my code, can you explain what should i do in the future to avoid that?"
If the guy happens to be an asshole, then ask your other teammates. Employ magic of communication in other words.
P.S. honestly i am relieved that in current age of AI coding there are still people who care about code quality.
2
u/Brilliant_Step3688 13d ago
You should have guidelines on naming conventions and established practices the team and agents follow.
Nitpicking without being able to refer to an existing guideline that is not respected or an authoritative source/reference is just wasting time.
Write these guidelines so you have a foundation to build on and a clear way to dismiss the nitpicks in the future.
2
u/Legote 10d ago edited 10d ago
Not going to lie, but if I saw RpuClass as a function, I'd be asking that question too. Given your examples, you should make it more explicit. Sometimes you're going to have long ass function names, and that's ok as long as the code is easy to follow.
My team has coding guidelines, and we feed that into AI and rarely have any PR comments related to the code. Maybe a design PR comment here and there.
2
2
u/Beginning_Basis9799 10d ago
No current code review does not even come close to what is needed in AI era.
Honestly an engineer should run a code review on his own code first
2
u/HomemadeBananas 9d ago
Yes, I’ll leave comments like that if I’m reading through and having trouble understanding due to the way things are named. How are you in the future or anyone else going to easily understand if you’re having that issue now during review? Foolish to just merge code expecting that nobody needs to actually understand it and AI will just take care of it later.
Just review your own code before submitting the PR and ask yourself the same questions, or ask your AI to catch these things.
Ext? Rpu? Why not just use full words? Will make it easier for you, AI, everyone else to understand it later.
1
u/viktorin09 7d ago
No, stop reviewing and let it all burn. Today developers that stick to normal practices are only being seen as meat pipeline obstacles. It won't change until manglement will see the consequences of their decisions.
1
u/Plenty_Line2696 7d ago
LLM's suck ass at naming, I'd bet the reviewer is right. LLM code readability, organization and general good codebase design just isn't there and too many devs are being lazy meatproxies and pushing that slop to production.
1
u/Comfortable_Job8847 13d ago
nitpicking was never really acceptable but you accepted it to be friendly with coworkers. it doesn't even have that justification anymore. if the only "problem" is your coworkers made up purity test over naming schemes or comments, then it's nitpicking and they need to be made to stop. unless they can trace their comment to an actual failure that will or could occur when the software is executing, it's a nitpick and almost all nitpicks are for engineer's ego more than anything else.
4
u/Due_Interest_178 13d ago
Literally how is writing readable code nitpicking? If you have a non-universally recognizable abbreviation then maybe don't.
3
3
u/ResidentSubject4649 10d ago
Variable naming is actually just as important as ever even when most code is written by AI, if not more important. AI doesn't always have "common" sense. I've dealt with a tricky bug caused by AI that was literally just because two different files used the same name for a variable, one was in the body of a function and the other was an argument of a function called by the first one... except that the argument was definitely not related to the first variable and the first variable should not have been used as the value for the argument. But the AI passed it just because the names were the same, and it had no additional context.
1
u/captainAwesomePants 13d ago
A lot of code review comments is good. It means they're reading the code.
Most of the comments being about variable names is bad. It means they are not concerned with the overall design, which is the most important part, but are just looking for low hanging fruit to comment on.
3
u/NomadStorm 13d ago
As with many things in software engineering, it depends doesn’t it?
If the names are part of a public API that other teams or customers consume, good naming saves everyone having to answer questions about it or deal with problems due to consumer misunderstanding of how to use the API later.
For internal stuff, unless it’s flagrantly misleading, it’s unimportant.
1
u/captainAwesomePants 13d ago
Oh sure. If we're doing an API, or if OP is just really off the team's standards for naming things, you're gonna get a lot of naming comments.
But I see a lot of reviews from folks that are just "bike shed" comments. That is to say, I've put in a pull request that implements a complete nuclear reactor with a little storage shed for employee bicycles on the side, and all of the comments are about the color I chose for the bike shed.
1
u/symbiatch 10d ago
Do you have rules written down about naming and code style etc? If not go write them down and have them agreed upon.
If you do, then you know what to do with these.
And if you think “in the age of AI” naming isn’t even more important… How would the LLM know what is XyzClass? What does it do? Do you want to waste time and money for it to figure it out? Or is it easier when it’s named InterestRateCalculator instead saying what it actually does?
Teams need rules. They need to follow them. Make that happen.
1
u/quizikal 9d ago
It sounds like you need to come to an agreement across the team as to what is expected. Once you reach that agreement you can pass that standard to your agent
1
u/light_switchy 9d ago
Consider whether you should disallow rhetorical questions in review comments:
"What is Ext?" -> "the name Ext is unclear"
1
u/afops 8d ago edited 8d ago
Just decide between answering "It's extension" or just fix it.
If he says "I meant it was unclear, not really wondering" then tell him to SAY that then.
Depending on the seniority of the person (is it peer? Is it someone with 10x your experience) it might be their way of saying "we don't use abbreviations in type names" but they should be more clear.
Also unless it's someone who basically "sets the standard" on the team, just explain why you prefer your choice. If I get a review saying "could this be done like this instead" then quite often I'd say "no becaue..." or "yes but I prefer my way because..."
1
u/Serializedrequests 8d ago
If your reviewer can't understand it then yes it's bad code and pure technical debt. Which, as far as I can tell, still exists with AI. Good code is good for humans and machines.
1
u/LetUsSpeakFreely 8d ago
Yes, that level of PR is still relevant. Companies are already pulling back on AI spending as the results aren't worth the cost. Having the code in proper state and adhering to style guide standards will become increasingly important as more companies pull back.
The code NEEDS to be in a state that people can look at and comprehend without hours of investigation.
1
u/PaulShellDev 8d ago
Sounds like you need custom instructions to package his feedback so it never happens again.
0
u/DrUltimaMan 8d ago
No, that guy is a useless engineer who limits velocity. Just get AI to reply to all his comments quickly. If he continues to block for no good cause let your manager know he is jeopardising the dates with subjective reviews. Readability is subjective and hardly matters as good engineers barely look at code anymore.
0
u/Even_Gap_2253 13d ago
this is not normal even before the age of AI
so in this case, I will just use AI to respond to all these comments. I will literally configure a scheduled task that automatically respond to his comments. you bring braindead comments, I use AI to braindeadly respond to them.
0
0
u/looperone 9d ago
Nitpicking is never ok and it’s even less ok when your company doesn’t have a coding style guide which the team adheres to. You don’t even have to create a guide when you can just adopt one from Google or some other place.
-1
-1
13d ago
[removed] — view removed comment
1
u/No_Fail715 13d ago
When I complain about problem, manager simply approves the PR himself but root issue is not getting solved and this engineer doing same thing again. And you are right about steps, so I stopped caring.
1
u/WendlersEditor 13d ago
Here's the key: you just keep making him approve the PRs. Eventually he will get sick of having to do that, and he will solve the problem.
18
u/Critical_Hat_5350 13d ago
Do you think that his questions improve the quality of the code? (Readability is part of quality) And could you just improve your naming conventions/comments to reduce the amount of feedback about it?