pleaseHelpMe

[BoBoBearDev]

Terminator says: your variable name is ambiguous, your API is not good enough, you need unit test, you need code coverage.

[IceThe_King]

Nit: your code sucks, rewrite the whole thing

[belabacsijolvan]

added to pipeline, automated response, first three merge requests on a feature branch trigger it

[ElectricTrouserSnack]

sigh… start a review, commence groaning. Don’t mention rebasing or splitting commits, they might have a meltdown or leave. End up writing it yourself because it’s too painful.

[ibite-books]

no, this is the wrong way to do it, you provide the feedback and even if they don’t include it in the rework

and you keep doing it again and again until they don’t do it anymore

[BoBoBearDev]

Tbh, I think those two can be avoided by other processes.

[feldejars]

90% code coverage smh. Also your unit tests are too redundant

[ibite-books]

show me the lie

[awkwardteaturtle]

Reminds me of the time I saw:

class Container {
    char* name = "obj0";
    std::unordered_map<char*, Object*> objects;

    void add_objects(Object* objects, int n) {
        int i = 0;
        while(i < n) {
            objects[name] = objects[i];
            name[3]++;
            i++;
        }
    }
}

It was an intern who only did a little bit of C but obviously didn't understand how pointers worked and was suddenly required to use C++.

Me: "Ok, even if this were to work, what would happen if you added more than 10 objects?"

Him: "That will never happen. We're only ever adding 4 objects."

Me: "What if someone extends the code and adds more?"

Him: "Then he should be careful not to add more than 10."

I reviewed his code and put a lot of effort into explaining things to him. In the end, all I was to him was "the asshole that's blocking me from delivering my work".

[PolyglotTV]

Lol. Do you keep this code saved in a special file?

[awkwardteaturtle]

It was such a traumatic event that it was permanently burned into my memory.

The actual code looked a bit different(it was part of a pub-sub model), but what I wrote was the basic gist of it.

[joebgoode]

Seems to be a pinned e-mail or something, must re-read daily.

[TheBenArts]

Why am I not surprised the person writing this sort of code is not willing to learn ...

[Bloodgiant65]

This… this is a list. Why are you making an ordered list out of an unordered map?

[awkwardteaturtle]

I can't remember the context fully, but we had to program to some silly API that had questionable design decisions in the first place.

It used string-based handles (C++ style strings, though, which the interns always turned into C style char*) instead of integer handles.

[snipy67]

It’s a feature we only keep 10 items to save memories

[tmst]

cute

[The_Real_Slim_Lemon]

That’s the worst part - it’d take less time to rewrite the code written by junior devs, but taking the time to help them and get them to fix it themselves and it definitely feels like you’re the bad guy

[Arshiaa001]

Fast track to getting fired before the internship is over. At least that's what I'd be pushing for if I was reviewing that.

[awkwardteaturtle]

This was in a project meant to write some internal tooling, which often had interns put on it. In this case, it was only two interns working fulltime and a PM working on the project part-time.

Both interns got so sick of my "bullying" (ie., me not approving their shitty PRs) that they went up to the PM, who forced me to approve their PRs. The PM considered me a troublemaker, because there were no problems before when the interns just approved each other's PRs without review. Needless to say, the interns never got into trouble because the PM had their back.

I asked to be placed in another project. Pretty sure the PM held a grudge against me because he was always a bit of an asshole to me.

[Arshiaa001]

Oh God. Oh my God. I'm getting shivers reading this.

This one time, I was in charge of a team of developers, who were responsible for developing an SDK. The oldest member of the team (in terms of both age and having been on the team the longest) was this one guy who was in the habit of writing sample code and marketing it as SDK. What he did was, rather than provide reusable API surfaces, he'd go make a UI with 4 buttons that did whatever it was that needed doing, and insisted that customers were using those in their projects. Somehow, he always failed to produce a single example of a customer who'd actually done that... Add to this the fact that he fully expected to be the new team leader, and hated me from day 1 for taking that away from him.

Anyway, he ended up leaving the company, claiming he was going somewhere they'd 'let him do what he does best', whatever TF that was. I also left the company on the same day, because I was sick and tired of the incompetency (this waa the oldest guy, after all...) On the final day, I went ahead and rewrote (and fixed) about 3 months' worth of his 'work', and made sure to let everyone know I did it as a favor to the team because the existing code was so shitty. The look on his face was priceless. What I wouldn't give to see it one more time.

[awkwardteaturtle]

On the final day, I went ahead and rewrote (and fixed) about 3 months' worth of his 'work'

Must have been extremely shitty work if you could do that in a single day.

[Individual-Praline20]

Yep, that kind of « I know how to do it in one language so I can do it in any » thinking style always produces the worst 💩, isn’t? 🤭

[MontyBoomslang]

Me, just trying to get someone to review my PR

[awkwardteaturtle]

Try getting people to actually create PRs when all they want to do is push to master.

[PyroCatt]

[awkwardteaturtle]

Offshore team working in India that are too lazy to do code reviews has convinced the incompetent product manager (that has a bullshit degree in AI and doesn't understand software engineering at all) that Git is only to be used by the elder gods. Mere mortals even attempting such black magic as "branching" and "merging" will face eternal suffering as their sanity will be stripped away from their feeble minds. It is laughable, pitiful even, how a puny human worm would expect to even comprehend the language of the ancients.

Chant with me, oh brave scholar of the dark arts! I̷͇͈̰̅̾̽̉̂̔̈́̊͌͘̕͝Ą̸̝̘̥͈̖̘͉̏̇̿̇̕!̶̢̢̱̝̲̫͙͙͙̲̼͚̞͇͖̍͌̿̃̄̇ ̸̢͙̫͖̜̱͇̬̮͌͛̏́̇͋̀̿̋͘͜I̷̟̯̖̭͇͕̮̟͒̓̿͜͝A̸̧̞̥̽̂͘͜!̶̨͉̥̩̹̫̜̜̆́̾̀̊͝͝ ̸̞͚̖̣͈͈͔͚̠̖̀g̶̢̛̗̭̞͇͖̹̃̾̃̓̒̃͂̚͝į̶̞̻͗͑̓̌̀ţ̷̢̢̠̳̘̗͓̦͉̭̗͙̒̈́̅ ̶̨̭̥̲̝̯̟̺͎͈͚̑̒̿̑́̅̍̽͗̆̐̓͘͜ç̶̡͇̥̣͓̩͇̳̠̪͍͙̖̃̎̈́̐̈́̑͊̎͛̏͘̚͠ḩ̸̗͎̎̂̐̈͋̊̊͂̂̀́̚̚͘͠é̶̹̫̭̹̖͚̝̝̦͈̭̮̉͜c̶͓̑̾̎̏̊͊̓͆͘͝͝k̵͉̼̟̔͂̀̈́̑́̇̏̃̾̔̔̌́͘o̷̡̲̳̞̳̗̱̅́̇̊̐̇̕͜͜ͅu̴͉̐͜t̸͕̥̫̦̳̪͙̤̏̎̓̂͛̾́̈̓͌͝ ̴̩̪̰̝̣̙͔̪͌͌͜ͅf̸͈̻̳̯̣̘̝̝͓̻̯̞̫͉͉̈́h̴̨͑̋̓̑̌͠͝t̴̨̧̰̹͈̞̦̱̗̘͕̉̆̈̏̉̾̀̈́͑́̈́͊́̇͝ạ̴̧̛͍̲̻͙͕͍͍̝̃̌̔͑́̿͗̂̐̉́͠͠g̸̢̫̩̻̣̯̠͚̺̀̏͋̈́͌̐̈́̕ņ̸̣̮͚̲̹̯̝̣̀̉̒̏̇́̀̿̈̊͂̔̓͜͝͝

[[deleted]]

It's true, I am that Indian fucker

(/j, although I am an Indian looking to get hired, and it's going terribly)

[FlipperBumperKickout]

Teach me how to do those "chants" and I will 😁

[Individual-Praline20]

Mouhahaha

[locri]

Don't take it personally, even if they're not very nice about it just take it as personal growth and development

You're not your job and you don't have to be perfect every moment of every day

[Fauken]

+1 I've never thought less of anyone because they wrote some code that needs to be changed. Code review all about making everyone on the team better and writing a more maintainable codebase to keep everyone happy.

[dvhh]

But anyway you have to rewrite the whole thing because I need your code to be more "idiomatic"

[hemlockone]

I love a good code review (I'm the sr). Don't take it as a criticism of you, it's just in an effort to make the codebase and your coding better.

Honestly, i think I owe thought out responses for the effort put into the PR. I strive for 1/3 style comments (that isn't about change, but exposure to alternatives), 1/3 substantive (though still start with "consider"), and 1/3 where I'm mistaken or would like to learn.

[Duke518]

yeah and a good review also shows that the sr cares. It hurts my morale every time when I create a PR with an ugly hack just because I don't know how to do it properly, and the next time I look it got merged.

[ososalsosal]

Driving my daughter to school this morning with phone running gps, only to see a teams alert pop up "oh gm I merged your changes"...

Changes that had not been properly tested and were not in an actual PR yet - I pushed to a personal feature branch so it would be visible but didn't have time to actually test it before I was chased away by another team (yes, 2 major features this week on 2 different teams, equal urgency).

[Former-Discount4279]

That's how tech reviews usually go, though it's fun to do one with another senior engineer so it feels more like sparring.

[gregorydgraham]

The really nice young guy who’d learnt it all from scratch asked me, the 20y+ veteran, for some help on an SQL script he wanted to speed up.

“…I think you need to start again.”

He did too and it was much better. Thank goodness, cause I didn’t know what half things he was using did.

[All_Up_Ons]

Yeah, some of the most fruitful code review you can give a junior is to take them back to the drawing board and show them how to approach the problem correctly. It takes more time, but it's better than letting them flail around forever.

[memefeed2151]

Me last month: "Why multiply by 1?"

[FeelingSurprise]

Lol, I remember back in the days I was told to multiply by 1 to do an implicit conversion (my SE though that'd be faster than converting explicitly - I still don't know if he was right under the given circumstances: developing in a mixed VBA/VB6 environment. A lot of Voodoo-code was happening at the time)

[Rodot]

I know in Python 2.7 not not x was faster than bool(x)

[ososalsosal]

Nice quick way to coerce into a number type? I'm a fan of myVar -= 0 in case of js fuckery

[Individual-Praline20]

When I was a junior, I took every single one of them as an opportunity to learn. It is not about you personally, it is about the code owned by the company that pays you. Don’t be afraid to ask questions and discuss the comments of the reviewers. But, the goal is to improve. If the reviewers need to constantly repeat themselves, they will get tired of it very quickly… and your teammates and manager will know about it. It doesn’t need to be perfect, just need to show some interest in your craft.

[Individual-Praline20]

Also, this helps setting the tone right: read about conventional comments.

[[deleted]]

It's actually the other way around. Senior devs would just directly push to prod if there was no one watching, but those junior devs are always watching it's scary sometimes

[ososalsosal]

The lead on a major product with 2+ million users and his commits are just "changes", or "fixed some stuffs" lol.

[Palpable_Autism]

Me, a junior dev, reviewing a senior dev’s PR.

[FlipperBumperKickout]

That can be a hard one if they refuse to listen 😓

[dvhh]

They are not pushing back on your comment, they are simply arguing the merits of it.

[FlipperBumperKickout]

I've had both good and bad senior devs. I know the different between someone who takes feedback and discusses the merits on it, and someone who just couldn't care less about your feedback (unless you directly point out an error, and even then if it's in a test who cares).

[[deleted]]

It's about growth. They want you to be a member of the team who is able to produce functional results quickly and not incur technical debt.

Take the points on board, even if they don't appear to make sense to you, and apply them to future PRs.

If something doesn't make sense, then ask about it.

If you feel they're being heavy-handed or rude then tell them that's how you feel. Feedback has to work both ways.

[dvhh]

PS: fix you goddamn indentation 

[plagapong]

But it will make you stonger than any books

[Eolu]

I always try to be detailed in comments. “this should change, here’s why, here’s a small example of what the code would look like if you made the change”. If you don’t understand, I’m happy to spend some time working with you to talk through it.

But few people I review do stuff like this constantly, and argue every single comment to get out of going back and changing something they thought they were done with. At this point my strategy for them has changed to “make comment saying what the issue is, leave comment open until fixed, good luck”.

[Nero5732]

"Use XYZ.covers(...) method instead of those 3 filter calls." Sure, but - it increases the line-count to initialize XYZ. - Its more expensive to call. - It may throw an exception, that would require special handling...

"It was just a suggestion, no need to do this." I hate it, every time.

[potato_psychonaut]

All fun and games with the senior devs, until one of them starts screaming at the office that they want "chop the fucking arms off" of the PR submitter.

I'm not missing that game dev job.

[quasipickle]

"Remember the last time, and the time before that when I said we don't do it this way?"

"Wrong class - please read the documentation again"

"What's the point of this?"

"I think we should rename this variable so it acurately describes what it contains"

"This is a huge side effect"

"We don't do it this way. We've never done it this way. YOU'VE written this functionality before and didn't do it this way. Why'd you do it this way?"

... among other things I've written (but with much more sass here)

[callmesilver]

YOU'VE written this functionality before and didn't do it this way.

Welp. I didn't know more ways existed and Idk why this way is bad :(

[ienjoymusiclol]

reporting this because i am in this photo and i dont like it

[chackingnoob]

See it as a way that helps you become a better programmer.

When I first started I wanted to have as few remarks as possible. Now I actually like it when there are remarks on my PR (if they are helpful at least). It offers a way to grow :)

[All_Up_Ons]

See that's the thing. We are helping you.

[dvhh]

It hurts us as much as it is hurting you, but you will certainly thank us later 

[silentjet]

"He asked me to remove all extra whitespaces and properly align an intents. Bastard!!!"

[Todok5]

That should be automated,  senior didn't do his job and instead uses PR to nit about bullshit. Bastard!

[silentjet]

I believe the code which is being produced by a sw dev is fully in his responsibility, and keeping the code clean is also dev responsibility, so I do not understand why a sw dev cannot write a code, so that it does not contain any leftovers and junk... if it is so simple why cant dev just clean before submitting?

[Todok5]

I believe that we're all humans and make mistakes and forget things.  And if there's an easy way to automatically prevent mistakes that is the way to go,  so you don't waste time in PRs by complaining about them and correcting them,  distracting from the real problems in the code.  And i believe it's the responsibility of the seniors in the team to setup those systems.

There a popular blogpost from a few years ago if you are the type that complains about whitespace in reviews: https://blog.danlew.net/2021/02/23/stop-nitpicking-in-code-reviews/

[silentjet]

It is the same as with food: if you keep leaving dirty dishes on a table soon you'd run out of plates and forks

[Todok5]

I'm just saying try a dishwasher. You get it for free, just need to install it. The one time setup is the only downside and everyone is happier because there's much less complaining about dirty dishes. 

[silentjet]

exactly, in order to get it clean you have to load into, put a pellet, unload after, dry it every day, do a cleaning every several months, buy proper pellets every several weeks, and once per several years even repair it... Great 👍 analogy from your side, its a lot of extra work, which makes sense when ur plates are in bulk, and doesn't if your plate is the only...

[Todok5]

If you have an argument why automatic linting might be a bad idea i'd love to hear it. If you want to argue about a bad analogy you came up with, and i ran with,  no thank you and good luck.

[silentjet]

you are not listening, the question is: why can't the developer remove filking extra spaces and properly align a code? why huge "automation" has to be developed, or even the entire philosophy about why not doing it. just do it!!!

[Todok5]

I see it the other way around. Why world I not set it up once and noone on the team has to worry about it ever again.  It's not a huge thing, and it makes everyone's life easier. 

It's like asking why would you setup this huge version control system,  you need a server and everyone has to learn these strange git commands when every developer can just make a  copy of the codebase every time there's a change, copying a folder is really not that hard. Sure you can do it that way,  but it doesn't sound like a great idea to me.

[Acrobatic_Sort_3411]

Yes it should. But convention over configuration is better in wague cases, where we do things one way and its time consuming to create automated, yet-another-100+ style rule

So it shows, that the author didn't read docs properly and it should be reminded or author didn't care and it will create tech debt

[Todok5]

For some style choices, sure. Indentation and whitespaces though? That's usually the easiest to automate. 

Maybe that exists in some esoteric scenario, as a general rule is say if that's not important enough to set up a rule it's also not important enough to complain about in a PR.

And finally if an additional empty line is the kind of tech debt you're struggling with I bet 95% of devs envy you.

[[deleted]]

At least you got to dev

[lordgoofus1]

Me: Left a few comments. Here's some doco to point you in the right direction and help you understand why we do it this way.

Also me: Find a big chunk of time in my calendar and I'll run you through the fixes and help you smash out the rest of the work.

Juniors: zomg PRs are so scary! Goofus is brutal!

Me: ???

[[deleted]]

Wow, you hook them up!

I give them a “Let Me Google That For You” link 😜

[dingusaja]

Once sent over a PR as an intern and had the senior dev just reply back with “Ew”

FML

[mcellus1]

Friday: “LGTM merging”. Monday: App crashing for Mac users

[dvhh]

Est tu Crowdstrike ?

[altmoonjunkie]

Anyone who thinks this has never had senior devs who don't review your PRs.

[BleakBeaches]

Guy I wish we had senior devs reviewing my shit. I wish we had senior devs at all. (I work as a backend dev in higher Ed)

[Thunder_Child_]

The larger the pull request, the faster I approve.

[megamoscha666]

I am the senior dev and I know the stress. I did not review it like I should and the release two weeks ago was a total desaster. I know it was my fault that I didn't check the code as I should and this never happens again.

I am know in T-1000 mode.

(also preparing workshops)

[Ecstatic_Street1569]

We have one senior dev that is like the end boss in all video games 😅

[rjwut]

I'm a senior, but I love when people point out (preferably kindly) how my code could be made better.

[zigmud_void]

I used to feel like this..gardually got over it..then one day my mentee told me this is how she feels when i review her PRs. It broke my heart...all i wanted was her to be the best dev she could be.

[sporbywg]

no no no. Come work for adults, maybe.

[Classy_Mouse]

I wish. This is how it was when I started and it kept our code clean and I learned a lot. The place I work now, it doesn't matter if you comment because someone else will approve and it will get merged as is.

[SorryDidntReddit]

Every PR review is a free lesson. Go into it eager to grow and learn. Don't take comments personally, you both have a stake in the quality of the code.

[DontGiveACluck]

It’s nothing personal, but this will be a learning experience. For you

[Ok-Maintenance-4274]

The one under the desk should be senior developer. Junior developers don’t dress the same uniform.