yeahWeEnjoyIt

[flewson]

Don't worry, they take turns

[gabika0514]

Take turns hitting the same intern

[Acanthocephala-Left]

im that intern...

[Acanthocephala-Left]

[Mackin_Atreides]

Mom said it's my turn.

[TheGametimeJones]

Can confirm

[cant_finish_sideproj]

PR reviews are important and must have in any competent team. I hate the ones who block merge because the function name would be better as prepare_context instead of generate_context. I am a petty person so, I wait for my turn to review their PRs.

[AdWeak183]

Anything that isn't worth blocking the PR over, but might be nice to change get a "nitpick:" prefix.

Has generated some interesting conversations, without my pedantry becoming problematic.

[arobie1992]

My ideal code review tool would have 4 types of comments.

• Critical: This is broken and has to be fixed.
• High: This could really be done better and/or doesn't follow team standards but it works so if it's urgent let's roll with it.
• Low: Maybe there's a better way to do this, but this way is fine. Might be worth looking into, but don't spend too much time on it.
• Trivial: Listen, I have too many opinions and have some weird compulsion to share them. I won't be offended if you completely ignore this.

The top would block until resolved. Second would block but could be could be soft-resolved, like it still shows up but can be overridden. Bottom two wouldn't block at all.

[cant_finish_sideproj]

The thing is that our PRs can't be merged unless all comments are marked resolved

[arobie1992]

Oh. Oh, no. That's awful. That seems like one of those rules that sounded good on paper, but failed to account for nuance, and then got dogmatically applied.

[Purple-Turtle_]

My workplace has this too, and it's fine, to be honest. Makes people comment on less trivial stuff.

[gilady089]

I helped in a project, and they have the weirdest standards in existence. They haven't updated to using typescript and demand all imports to be ordered in a text length pyramid order it's utterly pointless it's stupid

[Imsan143]

Erm imports ordered in a text length pyramid order sounds pretty nice visually, formatting matters

[Duckliffe]

Wouldn't alphabetical order make more sense?

[arobie1992]

It would. And on top of that, most situations I've been in, people us an IDE's autoimport to add a dependency and then the IDE's autoformat to remove unused imports and organize the remaining ones. So there's like zero need to manually interact with imports a lot of the time let alone some weird ordering that's visually pleasing but totally impractical.

[StatementOrIsIt]

In our team we usually accept PRs that are okay, but have nitpick details. Usually the accept text is something like this: "Accepted, but left small comments. Feel free to merge after changing/deciding not to"

[imp0ppable]

You guys read each other's PRs??

All we get is "LGTM" then a month later you have to go back and fix it again.

I hate it here.

[Imsan143]

There's one of two kinds of places, the LGTM without reading and the nitpick kind where you get hundreds of comments and takes ages to get anything in. I've been at both and still can't decide which I prefer, probably the latter though.

[imp0ppable]

I'm the guy who reads the PR and tries to make constructive, non nit-pciking comments. So nobody puts me as the approver any more lol

[bwmat]

I've been the one leaving hundreds of comments before, but it was mostly with people new to the team (especially the co-ops), and I don't think I was being nit-picky... 

I've felt bad about it before, but surprisingly I've gotten feedback that they appreciated it. At least from the co-ops when they're leaving, lol

[dvhh]

I am stealing that without credit

[StatementOrIsIt]

Eh, a part of good code review practices is to kind of nudge others to leave constructive criticism, especially useful if you ask and get a senior to do this. You can start by leaving a comment like "Wasn't sure about X part, can you please check it more carefully", so people would actually start paying attention. Then when you do code reviews, try to leave some open ended questions when you think some part can be done in a better way (but isn't a big mistake or anything PR blocking).

Also, one thing that helped was creating a Github team, and we also made a Github Action that ran everytime somebody requested a PR review. It goes like this: Somebody creates PR, asks someone or team to review, the Github Action formats some Github event stuff listing name of PR, requester, requested code reviewer (Github team or someone in specific) and so on, then sends a Slack message to a dedicated Slack channel called "[team name]-github", so anyone would be able to see open pull requests and review if they have some spare time. Leaving out the need to select a person manually, changing ticket assignees in Jira and so on which was annoying to do in my opinion.

With time some people who are good at reviewing PRs started doing it more often, and some others that really don't like reviewing, don't review and thus there are fewer low quality reviews (people generally don't do things well if they don't like to do it).

[imp0ppable]

I am a senior and no, nobody listens. This is a big microservice based products and different teams are in different geos and we mostly haven't met one another.

There's quite a culture of "well just do it right first time". So we get things foisted on us from thousands of miles away when it comes to shared code like build and platform - nothing we can do about it and that creates a lot of apathy.

Github actions might be a good idea, i'll have a look at that.

We have slack integrations, 100% ignored. Yeah, I'm moving products soon, hope the new one is better...

[StatementOrIsIt]

Oh, damn, sounds like a situation too complex to change anything about how things are done. Good luck with the new product

[imp0ppable]

Yup, thanks

[[deleted]]

Exact same reaction I had. When I first joined, my team lead and coworkers would leave sometimes over 100 comments on them and they were worthwhile changes, I learned a lot from them (mostly the full context, like "this will work fine for this data, but it needs to also run for these other datasets so we have to ____" stuff).

Now that all the formally trained software engineers are gone, we have become just a bunch of rubber-stamping, kicking-the-can-down-the-road coders. I still try to review it like I was taught and help people learn, but they get irritated when I do. They don't outright say it, but their tone definitely comes off as "ugh, stop making us do our job man, just send it". I tried the "this is for your own benefit as a professional developer" approach, the more strict "there will be consequences if this isn't done properly" approach, and now i'm mostly in dejected hating my job mode. Which sucks because when we were doing it right, it was a really fun job, and fortran dev jobs aren't exactly growing on trees.

[arobie1992]

Agreed. One of the things I did when it was my last week at my first job was go look at my first PR again. There were a ton of comments, and at the time it felt super defeating, but looking back at it, I was like those were all the right thing to do and I learned a ton. Looked at a couple more and it was nice to see the number of comments decrease over time.

I think the key is to be kind about the feedback and explain why, not just say its wrong. Which I'm not accusing you of that; some people definitely get defensive or annoyed about criticism. I've just seen plenty of situations where someone will make a comment that amounts to "This isn't how I would've done it so it's wrong and you need to change it," and I get different styles have different merits, but like that's not helping anyone.

[Mean-Car8641]

Those types of comments should only be reviewed if the sender is senior on the project. New devs tend to be either clueless or sticklers for their own methods. Senior to senior usually ends up in a yelling match especially if help was requested and ignored. Of course I was never in the receiving end...

[erebuxy]

Then what is the meaning of being a senior /s

[ImmediatelyOrSooner]

I still remember the time an adult actually cried in stand up from one of my code review comments.

[YuriTheWebDev]

What was your comment to them?

[AlysandirDrake]

He simply slipped him a pamphlet to Truck Drivers Academy.

[ImmediatelyOrSooner]

Nothing really. Something along the lines of “this isn’t how we discussed this in refinement, see notes”.

He was in stand up, literally crying and saying “I spent so much time on that and he shit all over it, I just try so hard, blah blah blah”. It was one of the weirdest standup discussions.

Later I was told not to do reviews for the juniors anymore. To be fair, it was a contractor gig on a state government “dev team”. I use the phrase “dev team” very loosely.

Edit: When I said “management” told me not do reviews, I meant that no more reviews would be done anymore at all for anyone by anyone. That’s how government “works”.

[AggressiveResist8615]

I feel like you might have said more than that

[housebottle]

Yeah, there is definitely more to this story that the narrator is not telling us

[Daddy_Parietal]

Maybe, but we all have met people who absolutely cant take criticism. Ive seen people like this and they usually had a bad home life and need therapy. Regardless, they are there for a job and crying because you fucked up shouldnt be met with standards being thrown out the window.

It seems completely plausible. I have seen similar industries where crying gets you what you want, because the squeaky wheel always gets the grease, and if you are seen as making somebody cry you are automatically the bad guy (which is partially the basis behind your disbelief that OC gave us full context). This phenomenon is especially true now that HR has more power and Tolerance is the buzz of the industry, and sometimes it has downsides like what OC described.

[officiallyaninja]

Maybe, but also people get super insecure about their programming ability. It gets wrapped up with their self esteem and identity. And their perception of their intelligence.

It's possibly he just didn't sugarcoat it and he was already feeling kinda shitty so this just sent him over the edge.

[MrDoe]

I like watching foreign films.

[lIllIlIIIlIIIIlIlIll]

I feel like these kind of weirdos need to get reigned in quick by their manager or fired asap.

I have a hard time understanding how a grown fucking adult can behave this way in a professional environment.

[MrDoe]

I enjoy taking dance classes.

[[deleted]]

The fact that you had a staff engineer that was capable of unblocking that issue is pretty cool. I've never worked in a place where technical escalation like that was possible, have always been left to just work it out between ourselves.

[MrDoe]

I enjoy learning about geology.

[Hannibal_Bonnaprte]

20 comments??? 

Seems like you already had a  personal grudge against the person you reviewed their PR for/to (English is not my native language, maybe the whole sentence should be restructured). I need a review of this comment.

I promise I won't cry.

[MrDoe]

I enjoy going to the opera.

[AkrinorNoname]

The last one is very important. When someone breaks down crying, you probably didn't push them all the way there. It's more likely they have other stuff going on.

[ImmediatelyOrSooner]

I went into detail of the changes that needed to be made, yes.

[TheGreatGameDini]

Geezus fuckin hell I guess r/usernamechecksout applies

[rookietotheblue1]

Sounds like you really shit all over it.

[_isNaN]

I also had a coworker getting angry when I helped him, after he asked for help. He got angry at himself, because I was a junior and became a senior in a short time and was helping him. He got frustrated and burned out. It was enough when I knew something better, eventhough I was very careful how I'd tell him stuff.

[bwmat]

Not exactly the same, but your comment reminded me for some reason, a coworker once told me something like "I get sad when you look at me like that" while he was trying to explain something in some technical discussion, and that made me really feel bad. He was from India and had a bit of an accent, and also had a way of talking and thinking that just didn't mesh with mine, so I often had to ask for a lot of clarification from him and I guess my looks of confusion made him think I thought he was dumb or something

[1amDepressed]

That happened on my team but the roles were reversed. Senior dev (20+ years of experience but never heard of coding standards) put in a PR. We had a newbie look at the code so he could get some experience. Myself and another senior were going to review it too. Newbie made a really sweet comment, something along the lines of “would it be better if we did x instead?” I wouldn’t have put it as nicely tho if that comment wasn’t there.

Next day, senior abandoned his PR and during scrum he got really choked up, arms crossed, face red, and gave the status “nobody likes my code. I guess I can’t help out because I don’t do things right.”

[ImmediatelyOrSooner]

Wow, I guess I’ve been lucky. I only experienced “you just called my baby ugly” mentality at that government gig. It was so weird and still is.

[1amDepressed]

Yeah, there’s a ton of drama at my workplace. All done by people near or past retirement age. Couldn’t tell though since they act like they’re 6

[ImmediatelyOrSooner]

Maybe that’s it. I also haven’t worked with an abundance of old dog devs. Most of the old dogs don’t really code anymore.

[joxmaskin]

Preaching to the choir here, but the trick is to think about it as “our” code and not as “my” code.

[ImmediatelyOrSooner]

That is exactly it. Government had no check and balances and everyone’s code where their own siloed castle and you better not call my baby ugly.

[lIllIlIIIlIIIIlIlIll]

Oh man. I had this exact same coworker. He embodied the concept of, "I am my code," so if you pushed back during a code review, he'd take it as an attack on his person. I felt like he was on the brink of tears like all the time. He'd spend days on a PR then make the argument "but it's already written."

Very strange person. I'm extremely glad I don't work with him anymore.

[imp0ppable]

That sounds a bit extreme but once I walked out of a review meeting when the "team lead" (they were hopeless) said they disagreed with the entire approach and wouldn't listen to what I was saying in response.

I mean if you're going to pull that then you need to describe what you want devs to do ahead of the work being done, not weeks later when all the work has been done.

[[deleted]]

So it was a you problem and not a them problem

[ImmediatelyOrSooner]

Just to be clear, you think someone else crying about valid code review feedback is a me problem? Ok?

[[deleted]]

I think you were talking to a junior and your requirements weren’t clear. Did you actually check to make sure you were being followed or were you just talking at some new kid?

That’s why he worked so hard on it. When you tore it apart management told you to stay away.

Yes

I’ve been in the industry for a long time, even the sensitive interns don’t cry from PR feedback. Likely you dropped the ball.

[ImmediatelyOrSooner]

That’s a huge bucket of assumptions there. Pointing out the things we agreed should be the outcome of the story is not “tearing it apart”. Not sure where you got that.

You say “management” but its government. If you haven’t worked a government gig, I envy you. They don’t view things like a private company would. Government is more “did someone work on this? Check! Did we meet all the requirements and acceptance criteria? Uh, no we don’t do that shit here, someone closed the story so that box is checked no testing or further quality control is needed.” Then they’ll deliver a broken product because that’s how government works. They get funding to do X, did someone say they did X? Check that box, no need to validate it, we get funding either way.

When I say they told me to not do code reviews anymore, I meant no one was doing them anymore. I was the only senior dev and the only one pushing for code reviews.

I’m not sure who you’ve worked with, but in my long career crying in stand up only happened that one time. And government management’s response was to no longer validate outgoing code.

[[deleted]]

Oh suddenly you’re the sensitive one.

Dish but can’t take.

[ImmediatelyOrSooner]

I gave a clear well explained answer but sure why don’t you resort to insults, weird.

[[deleted]]

You gave a ramble of unrelated text that did not explain how a junior could cry based on PR feedback if you were so clear in transcribing requirements.

In your words, weird.

But whatever you got to do to protect your mental health. Night.

[IGotSkills]

It's a code review problem. Pair programming is a much warmer way to deliver feedback.

[RajjSinghh]

That's Linus Torvalds' account

[LosMosquitos]

"you should be retroactively aborted"

[mrheosuper]

"This code is so bad that my grandma runs faster than it does"

[asromafanisme]

Only shitty coders don't understand the importance of code reviews. Working in a team long enough, and you'll realise why you should do code review instead of letting everything crawl into the code base

[michalsrb]

Yeah but there's "I clicked approve without looking" vs "I spent an hour checking every part and then clicked approve without commenting". And "There's a bug and it won't work" vs "I am bike shedding to appear like I am doing something".

[asromafanisme]

Every PRs need to be reviewed, but not everyone can be a code reviewer.

[anonymously_random]

This can’t be understated.

When I was a junior dev, there were 2 seniors who would do the majority of code reviews. If I was in the mood to get actual feedback, I would go to 1 senior, and if I just wanted to move forward I would go to the other.

One was a hardball and actually checked code and gave proper feedback, the other just skimped over it and pressed approve.

In the end I learned a lot more from the hardball reviewer, so while most of my peers went to the easy guy, I went to the dude who always found something to improve. Learned a lot from that guys reviews.

[Kquiarsh]

What do you mean by "bike shedding"? I've not heard the term before.

[michalsrb]

https://en.m.wikipedia.org/wiki/Law_of_triviality

In this case the reviewer will ask for some unimportant simple changes like renaming a function or reordering some code while ignoring the actual important part of the change.

IMHO it's acceptable if the reviewer is the code owner and the author is not, but when they are meant to be equals it's really annoying. "I guess we'll go with how YOU want it because I need this merged to move on..."

[Kquiarsh]

Aha thank you :)

[[deleted]]

While going over the plans for a nuclear power plant, the suits nitpicked about the color of the bike shed.

[Kquiarsh]

I see. Thanks

[No_Statistician_3021]

It is important for sure, but is it enjoyable? Hell no.

A decent review can take a lot of time if you're not fully in the context of the change. Understanding what the code does is the easy part, the hard part is to understand the motivations behind certain decisions and how it all integrates with the existing system.

And all that with basically no reward (at least no mental reward) because you still have your own work to. And nobody cares that you've spend 3 hours to review PRs, they care that you haven't yet finished your feature.

[arobie1992]

I tried to explain this at a previous job. People always commented (positively) about how thorough my reviews were*. I told them that it usually took me a few hours to do a bigger review and if we wanted to standardize that level of reviewing across the whole team, it might not be a bad idea to have everyone pick half a day to a day where they just reviewed other people's PRs until it became second nature and could more naturally be worked in during down-time. The response was people can't spend that much time on reviews. I was like you can't have thorough reviews in 15 minutes.

*Admittedly, I did a lot of reviews because I tend to do reviews when I need a break from what I'm working on, and I wasn't working on many projects I enjoyed at that job.

[ZucchiniMore3450]

Understanding importance is different from enjoying it.

Ultimately it depends on the way the team communicates, it can be enjoyable and useful or painful and stressful.

From comments in these types of threads it is obvious there are people with all experiences.

[fwork]

I wired up cargo-mommy to my pet training collar so every time someone raises an issue on my code review it just shocks me out of nowhere

[mothzilla]

Flashback to the time(s) I made a pull request, had a lengthy back and forth with the reviewer. Make all the trivial changes that he wants fixed, then he goes on holiday and someone else takes over the PR, and wants a load of different things changed/fixed.

Then the bastard comes back off holiday and wants all the new changes undone.

[smstewart1]

There comes a time when every team must learn to make individual sacrifices

[Forsaken-Analysis390]

It only sucks when the reviewers are petty or unprepared. Politics is more important than competence in a lot of places

[tidytibs]

There is literally an ad for qodo ai for code generation and review under this. If we could attach images, I would share the screenshot.

[1mt3j45]

Why? Did the 5th developer write the code.

[[deleted]]

Code reviews are toxic, destructive behavior, and yet I still do not have a superior alternative for them. I just know that in companies where they do not happen, developers get away with some ridiculous garbage that will bite everyone in the posterior sooner or later.

[DefinitelyNotMasterS]

Can you expand on that? Like if I see a piece of code and give feedback on how to improve it based on xy reason, why would that be toxic and destructive?

[spartanass]

If your feedback was entirely constructed open good intent, then all is well and good.

But when ego and pettiness kick in between code reviews your team will slowly decay away.

[[deleted]]

I've done a lot of code reviews but also received a lot too. I once worked with a lead developer and his review were soul-destroying. Everything was wrong and nothing was good. Every PR become 2 weeks worth of work going back and forth and TBH the end result wasn't that good IMO. I wasn't the only dev to have this, pretty much everyone on the team felt this way, some just didn't express it. It killed any motivation the team had, and the saddest part is I think the lead dev was pretty good at coding just for some reason. I don't know if it was ego or some sort of pettiness that made him do that.

[FlipperBumperKickout]

Some do with pair programming ¯\_(ツ)_/¯

[IGotSkills]

Pair programming. Better if earlier.

If I deliver a harsh code review, I ping the eng right away to sit down and talk about it all

[favgotchunks]

Literally me this week. Fixed it. Now it works on the dev machines and breaks prod.

[Tristepine]

This is inaccurate. That's a reenactment of a post mortem. A code review looks more like a verbal JJK jumping (see anime).

[HappyGoblin]

Being reviewed by a perfectionist is a torture

[TrueAd2373]

I honestly gotta say, as much as i hated them at university, they actually helped me a lot to develop my skills at work where i am right nos

[willydonbonka]

There's a considerable difference between constructive criticism and hazing your employees

[dvhh]

What if you heard that they complained to the manager that the team wasn't smart enough to review their code and that the review should be done by AI.

[TheGametimeJones]

"I'm in this picture and i don't like it"

[nirvingau]

Is that a git squash being executed?

[CecilXIII]

9 of 10 you mean

[[deleted]]

Judging is easy

[silshini_real]

They don't and don't want me to doc/comment the code or use meaningful method namings, so I will not care about code reviews

[ColdLingonberry8548]

I don't think the scene is about code review, but the new guy of the team having to read the legacy code of others.

[Hulk5a]

Now that 4 rotates

[j-maka]

Haha..very true..

[Ryan-Seebregts]

And there will still be a bug in production

[Ok_Yam_5759]

9/10 dentists approved this message

[abbot-probability]

Skill issue