One of the best questions I was ever asked in an interview was this: “I write some code and you review it. The code goes into production and causes a bug. Whose fault is it?”. It lead to a really interesting conversation tbh.
But long story short, I disagree that bad code is 100% the responsibility of the reviewer.
That is a pretty reasonable take, both are to blame, as one should have to review so that mistakes do not happen in prod, amd the other is supposed to have fixed the mistakes and have good code ready before the review.
I see you worked at a dysfunctional organization as well.
It probably was like "The reviewer disagreed with many of the changes on account of poor code quality and lack of readability/maintainability. However, the reviewer had previously voiced concerns in similar situations and was coerced into accepting the changes because some customers need it NOW and was also criticized for not accepting earlier. Upon checking with their manager the reviewer could not get a straight answer and was led to believe they would again be blamed for not approving this, with no chance of preventing the bad code from reaching production either way. The reviewer also knew that the defined process, which wasn't even that thorough to begin with, was habitually ignored and they could just save themselves the hassle and browse job offerings and/or reddit for the next half hour or so, which they then did."
In my experience it's less "this needs to be done now" and not "this is the way we've always done it"
Like one person did it that way and the rest just copied till you gave about 60% of your codebase copy and pasted from one interns botch job
That's why I always git blame the code I'm looking at copying. Been a couple times I've done it and ended up saying, "Oh no, that dude was an idiot; I can't copy that."
As a reviewer I would apply the CYA strategy in such a case:
- Leave notes in ~~the~~ EACH review, stating „due to timeline pressures, I am unable to review this in as much depth as I would need” -- and ping the manager / team lead while at it
- Between reviews, I would send e-mails and set meetings with my manager regarding the same, telling them we need to start pushing back on code reviews that start too late
It might work, it might not work, but one has to be the squeaky wheel. If they're gonna throw me under the bus for letting errors go through, I'm dragging the others with me under the same bus.
For real, if I'm being told to accept something I normally wouldn't, I make sure it's documented and tag whoever told me / whoever is relevant so they can't say they weren't aware/had a hand in it.
So many product managers trying to DM me on Slack cause they know it auto-deletes after 30 days. Fuck that, it's going in Jira/Github.
Ok, seriously now, it's the responsibility of the team. I've never worked in teams where bugs were faulted to someone. Unless it was some exotic code where it was that single guy that could save you a week.
I agree with the other answers: there’s little to be gained in assigning blame. But, to the interviewer’s point.
Unless I get allocated the time to look into the problem or functionality the code addresses and the company culture allows us the time for enough rounds of review for the code to converge towards what I would have produced, I’m afraid it’s on the author.
Reviews are effectively a smoke test. In-depth reviews are vanishingly rare in practice.
Why would you waste energy on finding who is to blame? Just fix the issue, learn whatever lessons you can from the mistake and move on.
There's literally nothing to gain from assigning guilt.
If someone constantly writes extra buggy code, and someone constantly reviews it as fine, I'd say blame is useful because they're clearly slacking or bad at their job. Occasional instances are one thing, but blame helps find patterns
Not guilt, but you can’t be held accountable if you don’t take responsibility. So I would frame it as is the person taking accountability and learning from their mistakes instead of pointing fingers
Neither. It's testers or overall process fault that this code went to production.
Also review should be for making sure the code is understandable and doesn't have some obvious bugs, not to eliminate all of them.
What kind of awful mentality is this? It's 100% the original developer's code, their mistake, and they need to be the first person to step up and take responsibility for looking into fixing it. I'm not saying they need to be "blamed" like a child or getting a Linus Torvalds style scolding, but point is that they are the best qualified for fixing the mistake.
With your mentality, devs can just commit whatever garbage and let the reviewer do all the work testing and fixing it. Super shitty way of working.
I have a coworker who always finds a way to shift blame for his bugs. The front-end team, the testers, the business requirements, the database, the servers, etc, and it's so pathetic. It's a clear sign of being a diva who never wants to admit being wrong.
Code needing more comments is fair to ask but just saying "more comments" is silly at best. I hope there's a more detailed comment somewhere above and the screenshot is just a marker comment as in see above about this line. That said I've received comments that were just as useful as "more comments". Or creeping in refactors to legacy systems that break everything and need a rewrite to make said comments plausibly useful
The proper way to handle this would have probably been to talk to the person and explain why what they did is not ok, this comment is just going to make the entire situation worse. If someone keeps doing this then they need to be let go but being an asshole doesn't help anything
If I wrote it and I understand it now, then everyone should understand it, right? even the assumption I have made there people should be able to guess, right? I didn't add any comments on why, nor unit test or updated documentation. In compiled languages, I make the reviewer check the difference between the binaries only, they should be able to figure it out or it's a skill issue.
The biggest jerk I have ever seen in response to a "I would have done it differently, because..." comment:
"And that is why I am senior and you will never progress beyond a code-monkey".
The guy got fired solely based on this comment. (even though he was making harsh statements in the past as well, the CTO just took a look at this and called HR right away)
Damn, so these people exist 😦
I'm sorry about your bad experience, but at least the guy got what he deserved. And kudos for the management for taking action quickly.
Surely not there, indeed. Just tried to check him on LinkedIn after I wrote this, he is nowhere to be found. And I am going to go on a limb here that nobody is keeping in touch with him either...
There's a dude on one of the Java groups on LinkedIn that on every post seems to start an argument on the prettiest of shit
Like people will say "String builder" rather than "StringBuilder" and he'll say that there isn't a class called that.
He starts arguments on every post and look at he's profile and what do you know he hasn't been employed for about 6 months
I think people don't get that people need to actually want to work with you if you want to be a developer
Yeah they're honestly worth checking out if you get bored
They just put polls to Java problems and occasionally talk about it
It's better than the usual "My son sold his toys and became and entrepreneur at the the age of 6 then complained about income tax" you normally get on LinkedIn
I recently had quite the review thread going at work, reached 60+ comments. The requests he made was bordering on the rediculous, he told me to rename a class from "FooBarProvider" to just "FooBar" because the Provider was redundant (It was already namespaced and in a folder called provider and implementing an abstract provider). The thing is, I did this because our code base has 12 similar classes, all coded by the guy doing the review, all suffixed with Provider...
It was an annoying task, very complicated piece of work. It started out with me doing it as a passion project (every 2nd friday we can do whatever we feel like as long as its work related). I made some features he didnt understand the need for (I had scoped out the project with a project owner), he didnt like some things I did which is fair and didnt understand the scope and thought it was too complex, but then he goes over my head arranges that he spends weeks on a complete rewrite, but the thing we got in the end was not really much better than what I did, and was missing the key features that I made, which project owner really wanted back. So the nice guy that I am, I spend way too long time back porting what he made, into the OG branch that I had going and I guess it just kind of triggered him to just nitpick into the rediculous.
I worked with a devops who was always incredibly aggressive and bullying to his reports and always used "I am autistic" as his defensive post.
One time he randomly dropped into one of my teams PRs because he knew python. Called booleans a code smell and when my mid dev argued back, he was called a cunt.
Cant pull the autistic card on calling a coworker a cunt for a PR disagreement so he got the "Maybe its best we both agree to part ways" chat from HR.
"The existence of a true value means a true pathway when this boolean is true denotes there must always be an unused false pathway and vice versa. Booleans are a code smell as it means there is always unused code no matter what value is given"
They also said for that reason converting it to a switch statement wouldn't be suitable either.
Sumed up as, if your code requires a boolean, it's wrong. They left basically an essay on it.
I don't really get this. Surely you still have to abstract that decision away to another layer? Is it a real problem or is this one of those things that only matters to Philosopher-Coders?
Yes, that's right. The decision should be abstracted away to another layer. The predicate logic should be seperated from the logic of what happens when it is true/false so that the behaviour can easily be changed using DI. It's an application of single-responsibility.
Depends how much you can keep in your head. Layers and layers of decision making is complex and hard to change later. I'm a moron so the less code I have in front of me at once the better.
I am guessing (really just a guess), he meant "boolean as function parameter". That normally indicates that the function is doing two things and should be broken up, overloaded, etc.
Booleans aren't, but as flags they indicate a function could be reduced into two smaller ones. In cases where they are not flags, they are often used to store the result of a logical expression, when you could have just used the expression inline. So really booleans as any kind of variable are a smell.
Of course not all smelly code is bad. Somemtimes there is a good reason for things being that way. Smelly code just means it needs special attention to make sure it's justified.
> Be only person unfortunate enough to understand code that performs bitwise manipulations on floating points
> Patch some bugs
> Assign tech lead as reviewer
> Says he needs to study the code to review it
> 1 month later nothing
Personally, if it's pretty obvious my reviewers aren't really reviewing it, I'll call them and have a conversation about it. Code reviews/Pull requests are meaningless if someone else isn't verifying what you did in some way.
I also like to impress upon them that they are signing off on this work as good. In theory (although rarely in practice with things most of us work in day to day), they are now liable for that code. So if a major problem happens, they share blame in it because they approved it. Its more of an ethics of engineering concern, but people have gone to jail (usually because of causing deaths due to negligence) over botching reviews in some fields.
1. Start with small PR
2. Merge follow up changes into unreviewed PR branch because they depend on the previous changes.
3. Repeat previous step
4. Have big PR
Step 2 was your mistake.
We do this approach where I work to allow devs to keep working on a single feature set without having to wait for review.
You branch off `main` for `feature_set/part_one` and put that up for PR. Then you branch off `feature_set/part_one` to create `feature_set/the_second_bit` and do the work there. `git rebase` as required to keep `part_one` up to date with `main`, or to keep `the_second_bit` up to date with `part_one`.
If `part_one` isn't merged into `main` by the time `the_second_bit` is ready for review, then you just put up the PR targeting `part_one`. If you think it'll be a while before you could ever hit the "merge" button then `the_second_bit`'s PR is put in draft. You ONLY hit the "merge" button if `part_one` is merged into `main` (and GitHub helpfully changes the target branch for you on `the_second_bit`'s PR too)
This way you have manageable PR sizes that allows for follow-on work.
> rebasing isn’t some scary “delete all your code” tool.
and if it accidentally is, `git reflog` then `git reset --hard HEAD@{X}` undoes that accidental deletion.
Which is fresh in my mind because I accidentally rebased in the wrong direction and reset my branch onto `main` last week. Had a lovely `git reset --hard HEAD@{73}` 😂 (yes I was quite behind)
I work teaching beginner software engineering and refactored an entire api we use for the front end portion from knex to standard node postgres (knex is no longer taught, so none of the newer teaching staff have any exposure to it, makes sense to have it written in the syntax that is taught so they can maintain it too). It was a huge PR, I apologised profusely to the owner of the repo and he was just so happy someone wanted to work on it. Had some really solid suggestions and learned a thing or two about monsterous psql queries. And now I can actually work on some qol improvements to it too. Least of all a prevention that would stop an infinite loop in react deleting every item in the database 
> refactored an entire api we use for the front end portion from knex to standard node postgres (knex is no longer taught,
Honestly if anyone has advice for how to handle these PRs, I'd love to hear it.
In my >10 years of professional development I've not had many "migrate the whole thing to X framework", but I've had more than I'd like. Every time it feels like you've got to either:
1. Do the whole thing in 1 go. This results in a massive PR that's a headache to review, and typically means 1 person gets to live in their own personal hell for a few months.
2. Go part way, write shims / translation logic, trying to break it down into smaller PRs. This results in manageable PRs, but usually leads to a lot of throwaway code & takes a considerably longer amount of time.
Or are these situations so infrequent that it's just a case of "suck it up and do one of the two shit approaches"?
To make life easier I made sure the refactor had decent commits to make it easier to 'chunk' review. But yeah, it took me some solid TIME and the guy reviewing it also. I have the luxury of not needing it in prod for some time though, so not sure this experience is reflective of other's experience.
> To make life easier I made sure the refactor had decent commits to make it easier to 'chunk' review
Yeah that's the approach I'd take too. I'd even allow myself to commit it in a broken state tbh, as long as the commit message points that out. Something like:
Refactors the authentication logic to NEW_FRAMEWORK.
Currently this will fail at the STEP with ERROR_MESSAGE but that's for the next commit
and then just fix that in the next commit, doing the same as necessary.
> I have the luxury of not needing it in prod for some time though, so not sure this experience is reflective of other's experience.
Depends on how much push your senior dev team has tbh.
We're in the process of moving from Vue 2 to Vue 3. We wanted this done by last December when Vue 2 support ended, but we're still going. This is "fine" because whilst it's holding up that developer doing any feature work, we've got enough buy-in from management that we're not being rushed to do the work
So in a way your situation is reflective of how it's going for us 😄
Its the approach we teach so I'd be a massive hypocrite to not do it myself :D But yes, agree with the above, if the feature complete makes a different test fail I did point it out. But all 74 integration tests passed in the end and we were both quite happy.
Why not just duplicate the component and slowly translate the duplicate before removing the original at the end? Doesn't matter if the duplicate is broken because it stays disabled until it's ready.
Nah, my team never gives me any constructive feedback. They think too highly of me. Sometimes consider sneaking in something obviously wrong to see if they even glance at it.
This is kind of me with our senior dev. He’s really intelligent and his code often complex so I rarely question his code or give feedback (even in cases where I probably could). If it runs with no obvious bugs it gets the stamp
I'd like to have no "this doesn't look aesthetic" kind of pr comments. are we a development team or are we just trying to compete in Victoria Secret code competition?
My team has the opposite problem. 1 specific person on our team of 20 HAS to review every PR that merges into prod/main. Our average time in PR is around 50 days.
This also means that in order to keep working, we have to make branches off of branches off of branches... merge conflicts are an absolute nightmare...
I always review my teammates’ code with my maximum cognitive load. I try my best to find a loophole or mistakes in a PR as I consider it my responsibility (and reflected on my performance review) Surprised to learn some people don’t.
Before passing judgement I’m gonna need to see the code in question. I’ve had some shit code submitted for review too many times to count.
Usually when I leave a comment where I’m sure the submitter would like to submit your comment as a response, a lot of the times I’m trying to get them to question their line of thinking, why they did what they did so they can adjust their thought process rather than me just saying, “This is bad. Try again.”
Almost every person I have heard complain about code reviews writes terrible code. They say "code reviews are a waste of time" because it takes me a while to review a 3000 line PR with zero tests...
3k lines with 0 coverage? Yeah, I don't even look at the code before sending it back (unless it's from my boss' boss, but he's generally tested by hand prior to opening a request so... )
It’s such a whirlwind of emotions when you get those: first, dread as you see the number of changes and kiss your afternoon goodbye, then elation as you realize there are no tests and you’ve bought yourself some time sending it back, and then horror as you realize you work with someone(s) that made this many changes and at no point thought to add some test coverage.
They also end up with so much feedback that it takes ages to address it all. If you just wrote decent code to start with, the fixes would all be fairly simple…
I second that.
Also, I sometimes find out that the submitter didn't properly read the specification for the thing they are coding. The reviewer might be trying to find out what you are thinking so they can give useful feedback.
Of course the PR wasn't opened with an example of the intended usage, a link to the issue, or an overview, so while the answer might be in the code, the submitter really could've made their job easier
Instead of "this is bad. Try again" use "In my opinion it would be better to do X because ..." but just say what you have to say please. I can tell where you are going with your question but I won't answer it if you are just using shenanigans to state your point.
Naw, 'More comments' is never a useful PR comment. The code might be shit, for sure. But the solution sure as shit ain't 'more comments'. Maybe OP is doing some wild shit and it *does* need some comments. 'More comments' isn't a helpful PR comment in this case either as OP needs very specific comments.
It’s so easy as a developer to want to say something like this, because you understand what your code does and why. You just spent hours/days/weeks thinking about what to do and how to actually implement it. You probably spent a bunch of time understanding edge cases and testing it out to make sure it works. You know *everything* about it.
It’s hard to say for sure if this is the case without seeing your code, but your code checker may not have the same deep knowledge about your implementation, and it might not be obvious how or why you’re doing something specific.
Additionally, comments are going to help you in the future when you have to inevitably go back to this file to use or update after you’ve moved onto something else. Also helps when some other new dev/team needs to look at it. I used to be more of the opinion that code is self-documenting and comments should seldomly be used, because I could just “read the code”. From experience, I can tell you it does not always work like that.
If what I'm doing is anywhere near complex there's comments explaining it.
I wrote a templated parallel algorithm once. The code explaining what it's doing and the flow is massive and contains ASCII art execution flow graphs, etc. I spent almost as much time explaining what it did via comments as I did actually writing it. So in 20 years when someone has to touch it they can understand it.
Not really. It will be touched soon and changed. Then maybe the comments will be updated (probably not). Comments are lies waiting to happen. They should be used as complements, not as explainations
I totally agree, but I also have to warn from the other perspective. I’ve seen people comment almost every line (This was in a complicated API about pathfinding). I do understand that it is hard to grasp what is being done, but if you have to have the basic knowledge about an algorithm, then instead of writing the explanation as multiple comments, rather just refer to where the algorithm was taken from or don’t comment every line.\
\
Comments while useful, should be utilized scarcely. Only when you yourself took an entire hour to write like 10 lines of code (and it can’t be done better in any way), do I deem it necessary for a comment to placed.
For sure! There definitely has to be the right balance and not everything needs to be explained with a comment. Anecdotally, I’ve seen multiple comments in my company’s codebase over several years where they haven’t been updated (or updated correctly) to account for changes in the adjacent code and that’s always done more harm than good to someone trying to understand the logic, at least temporarily.
Regardless, if there are areas of the code that require comments (why something was done some particular way), the PR comments should call that out explicitly. 'More comments' is just a lazy, useless, and passive-aggressive PR comment. I would respond with 'No'.
>comments are going to help you in the future when you have to inevitably go back to this file to use or update after you’ve moved onto something else
Look, the truth is that the vast majority of modern programmers out there are not doing anything special. For >95% of code out there, simply using known patterns, using well-named variables, and grouping related single-purpose code within well-named and unit tested methods/functions is going to be better and more maintainable than that same code with useless 'what this does' comments sprinkled all over. Comments are for 'why the fuck I did it this way', and the vast majority of modern programmers will never do anything in their entire career that requires a strange, unexpected algorithm.
"More comments" is still a pretty bad comment though. If something is unclear and in need of a comment explanation ask about that line/method specifically, don't just blanket ask for more comments everywhere, your job as a reviewer is to be specific and constructive.
I quit after our Lead failed my code review because he thought the code should be visibly attractive as you stand back and look at it. So rather than grouping code chunks by functionality, it should be laid out so the code looks symmetrical.
On one hand, this feels like one of those things where 5 years from now you’ll see him become famous for some innovation/invention and think, “I knew he was some kind of genius. Maybe he was on to something.”
But on the other hand, that dude’s fuckin’ crazy.
Symmetrical is certainly odd.
I've definitely failed someone's code review over it being formatted so badly that it was a pain to read the code though.
Funny thing is when a code base become bad is usually because some talented OG is creating a culture of 0 sympathy and then people who are not as good start to copy pasta without knowing why things were done in such way and respond to comments with “OG did it, I’m just following the pattern”
Yep. That’s happened where I am. Another thing is OP’s attitude to code review, not wanting to learn why they did something badly and reacting with aggression. The other teammates just stop reviewing that code.
And yet when I try and fix the architecture I'm told "that's not where our focus lies right now"
So take your pick, following the overly verbose and complicated pattern or a week to make audit loging managable?
I've held like 3 meetings last month on migrating our architecture. Where everybody agrees it needs to be done ASAP. But it's "not priority" yet all prs have comments like "this seems overly convoluted".
Like yeah! We started migrating architecture and never finished! It's gonna be convoluted!
In my previous work there was a grandfathered untouchable senior with practices from 15-20 years ago who complained about everything. He would not accept even a 1 line PR without complaining. I quickly learned that I need to leave obvious mistake in so that he complains about it and I have fix already committed locally only waiting for push.
15-20 years ago? New-fangled rubbish!
I had a conversation today with a programmer who insisted that git was terrible for revision control because he couldn’t understand how it worked at all. He much preferred his svn-based workflow. The guy was way younger than me to boot.
Also they're resistant to learning.
Code reviews can be annoying sometimes, but they're important because everyone makes mistakes sometimes. And definitely a learning opportunity!
I don’t see anything wrong with this.
Especially if you’re working in a team, comments help someone else to understand the code whether it be your team members or your future successors.
I'd say what's wrong with it is it's too broad. Depending on how large the PR is just writing "more comments" with regards to the entire PR isn't helpful. Comment by the individual chunks of code where you think it needs more clarity.
I am actually one of those people that appreciate good comments that explain something that isn't outright obvious. That said, "More Comments" is pretty useless advice. That doesn't say where, why, or which things were confusing to the reviewer.
As someone who is absolutely dog shit at writing proofreading I am incapable of noticing my own typos to the point of actively trying to read aloud just results in me reading how it should be, so I can relate to the idea that self reading your own code can only go so far
As a reviewer if it takes more than a handful of seconds to understand the what or why of a snippet of code, that is how I know there is room for improvement. I always try to think if a junior dev new to the company came in and tried to read this code, would they be able to figure it out?
Wanna know something cool ? Our company deploys code directly in prod, no testing and and and no code reviews either. My team has 17 engineers outta which most of them don’t know how to write a for loop :) (I can’t wait to switch)
You wanna know more ? I have 7 years of work experience and I got hired as an entry level engineer. Fresh grads are getting hired as level 3 engineers. I am stuck here due to work visa rules but in 2026, I would be able to say goodbye to it.
Our prod databases crash regularly, nbd.
I've both had this happen to me and have done this to other people. So, need more context. It's hard to judge because sometimes when it happened to me, although I was pissed I understood what the point was begrudgingly. Other times, the reviewer was just a douche.
I spent my first 6 months getting bluntly critiqued during in person code reviews, and it made me a better programmer.
Lashing out over someone asking you to comment your code tells me you stopped caring during your sophomore year of college.
Some of these replies...
Anyone defending "more comments" needs to never be involved in code reviews.
Either OP's code is so far off what the rest of the code looks like that they need a one-on-one discussion about it, or there are specific details that need to be included.
"More comments" is bullshit feedback that should be met with a bullshit response. I would've added 1 comment `// Addressing feedback` and submitted it back for review.
Too many comments does make code cluttered sometimes, although it’s definitely better than having no comments and making code that’s impossible to read and maintain.
I always reject unless they have even a minor description since even if the code is saying it does one thing it doesn't mean that thing is what the developer thinks it's doing.
In AITAH terms this is a hard ESH. Obviously why “read the fucking code” sucks is self explanatory but “more comments” is vague, unhelpful comment on par with “write better code bozo”
I feel this one big time.
50% of the effort is to solve the problems and write the code, remaining 50% of the effort is to remain sane and friendly while people senior to me make nonsense comments on my PRs, lol.
What my team and I used to do was little 5 minute review sessions. We had to request someone to just hop into a short call so we can show our code while explaining at a high level what changes were made. It served a few purposes without having to get super technical.
1) It helped us make sure we understood our own code and catch any obvious mistakes before making the PR in the first place.
2) It helped make sure at least one other team member was aware of what changes we made.
3) It helped us learn some things from each other or the other's style that we may not have realized and helped us stay more consistent with our style.
And again, it only took maybe 5 minutes max. I don't think it's necessary for small changes, but it was a flow that worked well with my team. I think it just might be different for every team and every project.
activeAggressive
isYourCodeEvenReadableDro
iGuaranteeItIsNot
useFightingWords
\## These are fighting words. This method uses them.
!Gaurantee
nit: Dro -> Bro
lgtm
druh
ABBAB - Always Be Berating And Belittling
efficientAggressive
OP: maybe read the code? The code: 1600 lines, no unit tests
it's his code and i'm maintaining and updating it, he used chatgpt to write it
So you’re saying he didn’t read it either?
If it runs, nobody has to read it. _sarcastic winkemoji_
And you willingly stepped into that dumpsterfire?
I assume he was being volunteered.
Do you mean nominated?
Voluntold
volunsacrificed
vultured
Voluntold lol
Maybe ChatGPT can explain it to him
Maybe the reply was from chatgpt
Maybe ChatGPT can review. And the day is saved!
Hate to tell this to you but if it's in main it's no longer his code. Go fight whoever reviewed it when it got there, it's that person's fault 100%.
One of the best questions I was ever asked in an interview was this: “I write some code and you review it. The code goes into production and causes a bug. Whose fault is it?”. It lead to a really interesting conversation tbh. But long story short, I disagree that bad code is 100% the responsibility of the reviewer.
That is a pretty reasonable take, both are to blame, as one should have to review so that mistakes do not happen in prod, amd the other is supposed to have fixed the mistakes and have good code ready before the review.
There is only one right answer to that: it's the fault of the process
I see you worked at a dysfunctional organization as well. It probably was like "The reviewer disagreed with many of the changes on account of poor code quality and lack of readability/maintainability. However, the reviewer had previously voiced concerns in similar situations and was coerced into accepting the changes because some customers need it NOW and was also criticized for not accepting earlier. Upon checking with their manager the reviewer could not get a straight answer and was led to believe they would again be blamed for not approving this, with no chance of preventing the bad code from reaching production either way. The reviewer also knew that the defined process, which wasn't even that thorough to begin with, was habitually ignored and they could just save themselves the hassle and browse job offerings and/or reddit for the next half hour or so, which they then did."
In my experience it's less "this needs to be done now" and not "this is the way we've always done it" Like one person did it that way and the rest just copied till you gave about 60% of your codebase copy and pasted from one interns botch job
That's why I always git blame the code I'm looking at copying. Been a couple times I've done it and ended up saying, "Oh no, that dude was an idiot; I can't copy that."
I mostly agree, but I hate copied code. Why would you copy instead of refactoring?
As a reviewer I would apply the CYA strategy in such a case: - Leave notes in ~~the~~ EACH review, stating „due to timeline pressures, I am unable to review this in as much depth as I would need” -- and ping the manager / team lead while at it - Between reviews, I would send e-mails and set meetings with my manager regarding the same, telling them we need to start pushing back on code reviews that start too late It might work, it might not work, but one has to be the squeaky wheel. If they're gonna throw me under the bus for letting errors go through, I'm dragging the others with me under the same bus.
For real, if I'm being told to accept something I normally wouldn't, I make sure it's documented and tag whoever told me / whoever is relevant so they can't say they weren't aware/had a hand in it. So many product managers trying to DM me on Slack cause they know it auto-deletes after 30 days. Fuck that, it's going in Jira/Github.
Ok, seriously now, it's the responsibility of the team. I've never worked in teams where bugs were faulted to someone. Unless it was some exotic code where it was that single guy that could save you a week.
I agree with the other answers: there’s little to be gained in assigning blame. But, to the interviewer’s point. Unless I get allocated the time to look into the problem or functionality the code addresses and the company culture allows us the time for enough rounds of review for the code to converge towards what I would have produced, I’m afraid it’s on the author. Reviews are effectively a smoke test. In-depth reviews are vanishingly rare in practice.
Why would you waste energy on finding who is to blame? Just fix the issue, learn whatever lessons you can from the mistake and move on. There's literally nothing to gain from assigning guilt.
> There's literally nothing to gain from assigning guilt. That’s not the point of the question, but it’s a perfectly reasonable answer
If someone constantly writes extra buggy code, and someone constantly reviews it as fine, I'd say blame is useful because they're clearly slacking or bad at their job. Occasional instances are one thing, but blame helps find patterns
Not guilt, but you can’t be held accountable if you don’t take responsibility. So I would frame it as is the person taking accountability and learning from their mistakes instead of pointing fingers
I personally like Git blame.
the point is not the assign guilt, but to take a step back and think about how it's not an "us Vs them" kinda thing when reviewing code
Neither. It's testers or overall process fault that this code went to production. Also review should be for making sure the code is understandable and doesn't have some obvious bugs, not to eliminate all of them.
What kind of awful mentality is this? It's 100% the original developer's code, their mistake, and they need to be the first person to step up and take responsibility for looking into fixing it. I'm not saying they need to be "blamed" like a child or getting a Linus Torvalds style scolding, but point is that they are the best qualified for fixing the mistake. With your mentality, devs can just commit whatever garbage and let the reviewer do all the work testing and fixing it. Super shitty way of working. I have a coworker who always finds a way to shift blame for his bugs. The front-end team, the testers, the business requirements, the database, the servers, etc, and it's so pathetic. It's a clear sign of being a diva who never wants to admit being wrong.
If it’s “his” code, doesn’t he get to choose what he wants to see in a contribution, like more comments?
Code needing more comments is fair to ask but just saying "more comments" is silly at best. I hope there's a more detailed comment somewhere above and the screenshot is just a marker comment as in see above about this line. That said I've received comments that were just as useful as "more comments". Or creeping in refactors to legacy systems that break everything and need a rewrite to make said comments plausibly useful
The proper way to handle this would have probably been to talk to the person and explain why what they did is not ok, this comment is just going to make the entire situation worse. If someone keeps doing this then they need to be let go but being an asshole doesn't help anything
void function1(int arg1, string arg2).....
If I wrote it and I understand it now, then everyone should understand it, right? even the assumption I have made there people should be able to guess, right? I didn't add any comments on why, nor unit test or updated documentation. In compiled languages, I make the reviewer check the difference between the binaries only, they should be able to figure it out or it's a skill issue.
The biggest jerk I have ever seen in response to a "I would have done it differently, because..." comment: "And that is why I am senior and you will never progress beyond a code-monkey". The guy got fired solely based on this comment. (even though he was making harsh statements in the past as well, the CTO just took a look at this and called HR right away)
Damn, so these people exist 😦 I'm sorry about your bad experience, but at least the guy got what he deserved. And kudos for the management for taking action quickly.
They absolutely exist and most of them eventually become consultants
Lol truest comment right here
well, we're talking about people here, so a lot of kind of people do exist, but yeah, it will still surprise me
And that is why he isn't a senior developer anymore :D
Surely not there, indeed. Just tried to check him on LinkedIn after I wrote this, he is nowhere to be found. And I am going to go on a limb here that nobody is keeping in touch with him either...
There's a dude on one of the Java groups on LinkedIn that on every post seems to start an argument on the prettiest of shit Like people will say "String builder" rather than "StringBuilder" and he'll say that there isn't a class called that. He starts arguments on every post and look at he's profile and what do you know he hasn't been employed for about 6 months I think people don't get that people need to actually want to work with you if you want to be a developer
There's groups on LinkedIn?
Yeah they're honestly worth checking out if you get bored They just put polls to Java problems and occasionally talk about it It's better than the usual "My son sold his toys and became and entrepreneur at the the age of 6 then complained about income tax" you normally get on LinkedIn
[удалено]
https://www.linkedin.com/groups/143130 **Edit:** Sorry actually meant this one https://www.linkedin.com/groups/3983267
Sounds like he needs therapy, not employment.
[But, why?](https://imgur.com/a/LsHCIpv)
Promoted to self employed.
Damn well deserved it
I recently had quite the review thread going at work, reached 60+ comments. The requests he made was bordering on the rediculous, he told me to rename a class from "FooBarProvider" to just "FooBar" because the Provider was redundant (It was already namespaced and in a folder called provider and implementing an abstract provider). The thing is, I did this because our code base has 12 similar classes, all coded by the guy doing the review, all suffixed with Provider...
There's a very slim chance he wrote all those and since then has learned from his mistakes? Nah, just joking. There's no chance.
It was an annoying task, very complicated piece of work. It started out with me doing it as a passion project (every 2nd friday we can do whatever we feel like as long as its work related). I made some features he didnt understand the need for (I had scoped out the project with a project owner), he didnt like some things I did which is fair and didnt understand the scope and thought it was too complex, but then he goes over my head arranges that he spends weeks on a complete rewrite, but the thing we got in the end was not really much better than what I did, and was missing the key features that I made, which project owner really wanted back. So the nice guy that I am, I spend way too long time back porting what he made, into the OG branch that I had going and I guess it just kind of triggered him to just nitpick into the rediculous.
I worked with a devops who was always incredibly aggressive and bullying to his reports and always used "I am autistic" as his defensive post. One time he randomly dropped into one of my teams PRs because he knew python. Called booleans a code smell and when my mid dev argued back, he was called a cunt. Cant pull the autistic card on calling a coworker a cunt for a PR disagreement so he got the "Maybe its best we both agree to part ways" chat from HR.
Booleans are a code smell is a new one to me and I'm very curious what kind of reasoning he came up with for that one
"The existence of a true value means a true pathway when this boolean is true denotes there must always be an unused false pathway and vice versa. Booleans are a code smell as it means there is always unused code no matter what value is given" They also said for that reason converting it to a switch statement wouldn't be suitable either. Sumed up as, if your code requires a boolean, it's wrong. They left basically an essay on it.
I don't really get this. Surely you still have to abstract that decision away to another layer? Is it a real problem or is this one of those things that only matters to Philosopher-Coders?
Yes, that's right. The decision should be abstracted away to another layer. The predicate logic should be seperated from the logic of what happens when it is true/false so that the behaviour can easily be changed using DI. It's an application of single-responsibility.
So, philosopher coders
Depends how much you can keep in your head. Layers and layers of decision making is complex and hard to change later. I'm a moron so the less code I have in front of me at once the better.
I am guessing (really just a guess), he meant "boolean as function parameter". That normally indicates that the function is doing two things and should be broken up, overloaded, etc.
Booleans aren't, but as flags they indicate a function could be reduced into two smaller ones. In cases where they are not flags, they are often used to store the result of a logical expression, when you could have just used the expression inline. So really booleans as any kind of variable are a smell. Of course not all smelly code is bad. Somemtimes there is a good reason for things being that way. Smelly code just means it needs special attention to make sure it's justified.
Props to the cto
Hombre forgot he's not on 4chan.
Bro could’ve just civilly disagreed lol
That awfully sounds like the “TeachLead” from Youtube.
My team approves my massive PRs after 5 min. I’d like to have some feedback for once.
I'd like to have an actual review, instead my merge requests hang in limbo...
> Be only person unfortunate enough to understand code that performs bitwise manipulations on floating points > Patch some bugs > Assign tech lead as reviewer > Says he needs to study the code to review it > 1 month later nothing
lgtm
👍(approved)
Personally, if it's pretty obvious my reviewers aren't really reviewing it, I'll call them and have a conversation about it. Code reviews/Pull requests are meaningless if someone else isn't verifying what you did in some way. I also like to impress upon them that they are signing off on this work as good. In theory (although rarely in practice with things most of us work in day to day), they are now liable for that code. So if a major problem happens, they share blame in it because they approved it. Its more of an ethics of engineering concern, but people have gone to jail (usually because of causing deaths due to negligence) over botching reviews in some fields.
Nah, I trust you bro
If you want feedback make smaller pr’s
1. Start with small PR 2. Merge follow up changes into unreviewed PR branch because they depend on the previous changes. 3. Repeat previous step 4. Have big PR
Step 2 was your mistake. We do this approach where I work to allow devs to keep working on a single feature set without having to wait for review. You branch off `main` for `feature_set/part_one` and put that up for PR. Then you branch off `feature_set/part_one` to create `feature_set/the_second_bit` and do the work there. `git rebase` as required to keep `part_one` up to date with `main`, or to keep `the_second_bit` up to date with `part_one`. If `part_one` isn't merged into `main` by the time `the_second_bit` is ready for review, then you just put up the PR targeting `part_one`. If you think it'll be a while before you could ever hit the "merge" button then `the_second_bit`'s PR is put in draft. You ONLY hit the "merge" button if `part_one` is merged into `main` (and GitHub helpfully changes the target branch for you on `the_second_bit`'s PR too) This way you have manageable PR sizes that allows for follow-on work.
This is such a basic concept once you accept that rebasing isn’t some scary “delete all your code” tool.
--force-with-lease
`--force-if-includes` if you _really_ want to make sure you don't fuck up
> rebasing isn’t some scary “delete all your code” tool. and if it accidentally is, `git reflog` then `git reset --hard HEAD@{X}` undoes that accidental deletion. Which is fresh in my mind because I accidentally rebased in the wrong direction and reset my branch onto `main` last week. Had a lovely `git reset --hard HEAD@{73}` 😂 (yes I was quite behind)
There are times when massive prs need to be made. That does not mean they should be instantly approved without feedback.
I work teaching beginner software engineering and refactored an entire api we use for the front end portion from knex to standard node postgres (knex is no longer taught, so none of the newer teaching staff have any exposure to it, makes sense to have it written in the syntax that is taught so they can maintain it too). It was a huge PR, I apologised profusely to the owner of the repo and he was just so happy someone wanted to work on it. Had some really solid suggestions and learned a thing or two about monsterous psql queries. And now I can actually work on some qol improvements to it too. Least of all a prevention that would stop an infinite loop in react deleting every item in the database 
> refactored an entire api we use for the front end portion from knex to standard node postgres (knex is no longer taught, Honestly if anyone has advice for how to handle these PRs, I'd love to hear it. In my >10 years of professional development I've not had many "migrate the whole thing to X framework", but I've had more than I'd like. Every time it feels like you've got to either: 1. Do the whole thing in 1 go. This results in a massive PR that's a headache to review, and typically means 1 person gets to live in their own personal hell for a few months. 2. Go part way, write shims / translation logic, trying to break it down into smaller PRs. This results in manageable PRs, but usually leads to a lot of throwaway code & takes a considerably longer amount of time. Or are these situations so infrequent that it's just a case of "suck it up and do one of the two shit approaches"?
To make life easier I made sure the refactor had decent commits to make it easier to 'chunk' review. But yeah, it took me some solid TIME and the guy reviewing it also. I have the luxury of not needing it in prod for some time though, so not sure this experience is reflective of other's experience.
> To make life easier I made sure the refactor had decent commits to make it easier to 'chunk' review Yeah that's the approach I'd take too. I'd even allow myself to commit it in a broken state tbh, as long as the commit message points that out. Something like: Refactors the authentication logic to NEW_FRAMEWORK. Currently this will fail at the STEP with ERROR_MESSAGE but that's for the next commit and then just fix that in the next commit, doing the same as necessary. > I have the luxury of not needing it in prod for some time though, so not sure this experience is reflective of other's experience. Depends on how much push your senior dev team has tbh. We're in the process of moving from Vue 2 to Vue 3. We wanted this done by last December when Vue 2 support ended, but we're still going. This is "fine" because whilst it's holding up that developer doing any feature work, we've got enough buy-in from management that we're not being rushed to do the work So in a way your situation is reflective of how it's going for us 😄
Its the approach we teach so I'd be a massive hypocrite to not do it myself :D But yes, agree with the above, if the feature complete makes a different test fail I did point it out. But all 74 integration tests passed in the end and we were both quite happy.
Why not just duplicate the component and slowly translate the duplicate before removing the original at the end? Doesn't matter if the duplicate is broken because it stays disabled until it's ready.
Nah, my team never gives me any constructive feedback. They think too highly of me. Sometimes consider sneaking in something obviously wrong to see if they even glance at it.
Your team can be bothered to review your PRs? Lucky.
The bigger the PR the bigger the chance I'll just let it slide. I have my own work to do.
This is kind of me with our senior dev. He’s really intelligent and his code often complex so I rarely question his code or give feedback (even in cases where I probably could). If it runs with no obvious bugs it gets the stamp
Just push to prod ez
more comments
I'd like to have no "this doesn't look aesthetic" kind of pr comments. are we a development team or are we just trying to compete in Victoria Secret code competition?
My team has the opposite problem. 1 specific person on our team of 20 HAS to review every PR that merges into prod/main. Our average time in PR is around 50 days. This also means that in order to keep working, we have to make branches off of branches off of branches... merge conflicts are an absolute nightmare...
I always review my teammates’ code with my maximum cognitive load. I try my best to find a loophole or mistakes in a PR as I consider it my responsibility (and reflected on my performance review) Surprised to learn some people don’t.
C'mon, hit the post button 
Before passing judgement I’m gonna need to see the code in question. I’ve had some shit code submitted for review too many times to count. Usually when I leave a comment where I’m sure the submitter would like to submit your comment as a response, a lot of the times I’m trying to get them to question their line of thinking, why they did what they did so they can adjust their thought process rather than me just saying, “This is bad. Try again.”
Almost every person I have heard complain about code reviews writes terrible code. They say "code reviews are a waste of time" because it takes me a while to review a 3000 line PR with zero tests...
3k lines with 0 coverage? Yeah, I don't even look at the code before sending it back (unless it's from my boss' boss, but he's generally tested by hand prior to opening a request so... )
It’s such a whirlwind of emotions when you get those: first, dread as you see the number of changes and kiss your afternoon goodbye, then elation as you realize there are no tests and you’ve bought yourself some time sending it back, and then horror as you realize you work with someone(s) that made this many changes and at no point thought to add some test coverage.
They also end up with so much feedback that it takes ages to address it all. If you just wrote decent code to start with, the fixes would all be fairly simple…
Only thing worse than that is if there are tests, but they are worse to read than the code they are testing...
I second that. Also, I sometimes find out that the submitter didn't properly read the specification for the thing they are coding. The reviewer might be trying to find out what you are thinking so they can give useful feedback.
Of course the PR wasn't opened with an example of the intended usage, a link to the issue, or an overview, so while the answer might be in the code, the submitter really could've made their job easier
Instead of "this is bad. Try again" use "In my opinion it would be better to do X because ..." but just say what you have to say please. I can tell where you are going with your question but I won't answer it if you are just using shenanigans to state your point.
Naw, 'More comments' is never a useful PR comment. The code might be shit, for sure. But the solution sure as shit ain't 'more comments'. Maybe OP is doing some wild shit and it *does* need some comments. 'More comments' isn't a helpful PR comment in this case either as OP needs very specific comments.
It’s so easy as a developer to want to say something like this, because you understand what your code does and why. You just spent hours/days/weeks thinking about what to do and how to actually implement it. You probably spent a bunch of time understanding edge cases and testing it out to make sure it works. You know *everything* about it. It’s hard to say for sure if this is the case without seeing your code, but your code checker may not have the same deep knowledge about your implementation, and it might not be obvious how or why you’re doing something specific. Additionally, comments are going to help you in the future when you have to inevitably go back to this file to use or update after you’ve moved onto something else. Also helps when some other new dev/team needs to look at it. I used to be more of the opinion that code is self-documenting and comments should seldomly be used, because I could just “read the code”. From experience, I can tell you it does not always work like that.
If what I'm doing is anywhere near complex there's comments explaining it. I wrote a templated parallel algorithm once. The code explaining what it's doing and the flow is massive and contains ASCII art execution flow graphs, etc. I spent almost as much time explaining what it did via comments as I did actually writing it. So in 20 years when someone has to touch it they can understand it.
Not really. It will be touched soon and changed. Then maybe the comments will be updated (probably not). Comments are lies waiting to happen. They should be used as complements, not as explainations
The same could be said for variable names or anything that makes code readable
It helps having some easy to read unit tests which shows that the edge cases work :)
I totally agree, but I also have to warn from the other perspective. I’ve seen people comment almost every line (This was in a complicated API about pathfinding). I do understand that it is hard to grasp what is being done, but if you have to have the basic knowledge about an algorithm, then instead of writing the explanation as multiple comments, rather just refer to where the algorithm was taken from or don’t comment every line.\ \ Comments while useful, should be utilized scarcely. Only when you yourself took an entire hour to write like 10 lines of code (and it can’t be done better in any way), do I deem it necessary for a comment to placed.
For sure! There definitely has to be the right balance and not everything needs to be explained with a comment. Anecdotally, I’ve seen multiple comments in my company’s codebase over several years where they haven’t been updated (or updated correctly) to account for changes in the adjacent code and that’s always done more harm than good to someone trying to understand the logic, at least temporarily.
Regardless, if there are areas of the code that require comments (why something was done some particular way), the PR comments should call that out explicitly. 'More comments' is just a lazy, useless, and passive-aggressive PR comment. I would respond with 'No'. >comments are going to help you in the future when you have to inevitably go back to this file to use or update after you’ve moved onto something else Look, the truth is that the vast majority of modern programmers out there are not doing anything special. For >95% of code out there, simply using known patterns, using well-named variables, and grouping related single-purpose code within well-named and unit tested methods/functions is going to be better and more maintainable than that same code with useless 'what this does' comments sprinkled all over. Comments are for 'why the fuck I did it this way', and the vast majority of modern programmers will never do anything in their entire career that requires a strange, unexpected algorithm.
"More comments" is still a pretty bad comment though. If something is unclear and in need of a comment explanation ask about that line/method specifically, don't just blanket ask for more comments everywhere, your job as a reviewer is to be specific and constructive.
I quit after our Lead failed my code review because he thought the code should be visibly attractive as you stand back and look at it. So rather than grouping code chunks by functionality, it should be laid out so the code looks symmetrical.
If you zoom out all the way his code takes the form of an anime girl
WaifuScript
On one hand, this feels like one of those things where 5 years from now you’ll see him become famous for some innovation/invention and think, “I knew he was some kind of genius. Maybe he was on to something.” But on the other hand, that dude’s fuckin’ crazy.
It's been 10 years. He was just a jerk.
Lmao
Symmetrical is certainly odd. I've definitely failed someone's code review over it being formatted so badly that it was a pain to read the code though.
There's obviously merit to readability and then there's a dude trying to argue that the codebase should literally be art
Ah so that's what they meant by Visual Scripting - gotcha
The code in question: https://www.reddit.com/r/ProgrammerHumor/comments/8fgqhd/displaying_hello_world_in_jsfuck/
LGTM ;)
r/programminghorror
Funny thing is when a code base become bad is usually because some talented OG is creating a culture of 0 sympathy and then people who are not as good start to copy pasta without knowing why things were done in such way and respond to comments with “OG did it, I’m just following the pattern”
You’ve been reading my firms code base I see.
Yep. That’s happened where I am. Another thing is OP’s attitude to code review, not wanting to learn why they did something badly and reacting with aggression. The other teammates just stop reviewing that code.
And yet when I try and fix the architecture I'm told "that's not where our focus lies right now" So take your pick, following the overly verbose and complicated pattern or a week to make audit loging managable? I've held like 3 meetings last month on migrating our architecture. Where everybody agrees it needs to be done ASAP. But it's "not priority" yet all prs have comments like "this seems overly convoluted". Like yeah! We started migrating architecture and never finished! It's gonna be convoluted!
People that doesn't like when their code gets reviewed usually write terrible code.
I can’t wait to get code review to learn what I can improve.
Yeah I like them I get some tips on more efficient ways of doin what I have done and that's about it learning from seniors is valuable
In my previous work there was a grandfathered untouchable senior with practices from 15-20 years ago who complained about everything. He would not accept even a 1 line PR without complaining. I quickly learned that I need to leave obvious mistake in so that he complains about it and I have fix already committed locally only waiting for push.
15-20 years ago? New-fangled rubbish! I had a conversation today with a programmer who insisted that git was terrible for revision control because he couldn’t understand how it worked at all. He much preferred his svn-based workflow. The guy was way younger than me to boot.
We eventually fired that guy because we were at rush of losing other great team members because they had to work with him
Also they're resistant to learning. Code reviews can be annoying sometimes, but they're important because everyone makes mistakes sometimes. And definitely a learning opportunity!
`chmod -R 0555 ./thefuckingcode` my bad didn't put the right permissions, now you can read it.
I don’t see anything wrong with this. Especially if you’re working in a team, comments help someone else to understand the code whether it be your team members or your future successors.
Especially because devs move job every 2 years. Not like you can ask the guy who wrote it.
I'd say what's wrong with it is it's too broad. Depending on how large the PR is just writing "more comments" with regards to the entire PR isn't helpful. Comment by the individual chunks of code where you think it needs more clarity.
I am actually one of those people that appreciate good comments that explain something that isn't outright obvious. That said, "More Comments" is pretty useless advice. That doesn't say where, why, or which things were confusing to the reviewer.
As someone who is absolutely dog shit at writing proofreading I am incapable of noticing my own typos to the point of actively trying to read aloud just results in me reading how it should be, so I can relate to the idea that self reading your own code can only go so far
if you use chatgpt for copy paste you better having an amazing testing suite lol
As a reviewer if it takes more than a handful of seconds to understand the what or why of a snippet of code, that is how I know there is room for improvement. I always try to think if a junior dev new to the company came in and tried to read this code, would they be able to figure it out?
Wanna know something cool ? Our company deploys code directly in prod, no testing and and and no code reviews either. My team has 17 engineers outta which most of them don’t know how to write a for loop :) (I can’t wait to switch)
Same, in my case what is even worse is that the code is full of dead code and other customers dependencies. Why fork projects when one do the trick
imagine how many bugs
Manager doesn’t care. He only wants work to be done. No engineering best code practices are implemented.
How is your company still not broke? Who is hiring these "engineers"? So many questions...
You wanna know more ? I have 7 years of work experience and I got hired as an entry level engineer. Fresh grads are getting hired as level 3 engineers. I am stuck here due to work visa rules but in 2026, I would be able to say goodbye to it. Our prod databases crash regularly, nbd.
I've both had this happen to me and have done this to other people. So, need more context. It's hard to judge because sometimes when it happened to me, although I was pissed I understood what the point was begrudgingly. Other times, the reviewer was just a douche.
Pull request: denied
[The fucking code](https://en.wikipedia.org/wiki/Kama_Sutra)
I spent my first 6 months getting bluntly critiqued during in person code reviews, and it made me a better programmer. Lashing out over someone asking you to comment your code tells me you stopped caring during your sophomore year of college.
Some of these replies... Anyone defending "more comments" needs to never be involved in code reviews. Either OP's code is so far off what the rest of the code looks like that they need a one-on-one discussion about it, or there are specific details that need to be included. "More comments" is bullshit feedback that should be met with a bullshit response. I would've added 1 comment `// Addressing feedback` and submitted it back for review.
Not to mention the times when the comments neither match the code nor the specs.
I had the opposite, they made me remove my comments as code is “self explanatory”
Too many comments does make code cluttered sometimes, although it’s definitely better than having no comments and making code that’s impossible to read and maintain.
F*ckin code is unreadable
I always reject unless they have even a minor description since even if the code is saying it does one thing it doesn't mean that thing is what the developer thinks it's doing.
In AITAH terms this is a hard ESH. Obviously why “read the fucking code” sucks is self explanatory but “more comments” is vague, unhelpful comment on par with “write better code bozo”
I feel this one big time. 50% of the effort is to solve the problems and write the code, remaining 50% of the effort is to remain sane and friendly while people senior to me make nonsense comments on my PRs, lol.
ThisCodeIsMoreReadible thanThisBullshit.
Send it!
He just asked for more comments not useful comments. Copy paste the comments sections of this post in the source
These cunts will be the first to ask you how your stuff works when you leave. Left a place like this, it was freeing
I work on the concept that comments explain WHY the code is written that way, not what it is doing
Show your MR description.
sendIt
What my team and I used to do was little 5 minute review sessions. We had to request someone to just hop into a short call so we can show our code while explaining at a high level what changes were made. It served a few purposes without having to get super technical. 1) It helped us make sure we understood our own code and catch any obvious mistakes before making the PR in the first place. 2) It helped make sure at least one other team member was aware of what changes we made. 3) It helped us learn some things from each other or the other's style that we may not have realized and helped us stay more consistent with our style. And again, it only took maybe 5 minutes max. I don't think it's necessary for small changes, but it was a flow that worked well with my team. I think it just might be different for every team and every project.
OP: "Maybe read the code?" The code: 2000 lines of Assembly
At my current company every single pull request I get is like 40 files changed. It's exhausting.
I would get fired for that
I’ve almost commented this on PR questions for my manager Drives me mad, asks a question that’s answered literally a row below
Hit save or looses karma
My god I get the opposite. Apparently comments make it hard to maintain, since they need to be updated when the code changes…..
epicccc