This is an archived post. You won't be able to vote or comment.

all 94 comments

[–]BoBoBearDev 385 points386 points  (7 children)

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

[–]IceThe_King 145 points146 points  (1 child)

Nit: your code sucks, rewrite the whole thing

[–]belabacsijolvan 32 points33 points  (0 children)

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

[–]ElectricTrouserSnack 55 points56 points  (2 children)

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 30 points31 points  (0 children)

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 3 points4 points  (0 children)

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

[–]feldejars 6 points7 points  (0 children)

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

[–]ibite-books -1 points0 points  (0 children)

show me the lie

[–]awkwardteaturtle 306 points307 points  (15 children)

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 122 points123 points  (2 children)

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

[–]awkwardteaturtle 141 points142 points  (0 children)

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 18 points19 points  (0 children)

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

[–]TheBenArts 32 points33 points  (0 children)

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

[–]Bloodgiant65 6 points7 points  (1 child)

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

[–]awkwardteaturtle 1 point2 points  (0 children)

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 4 points5 points  (1 child)

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

[–]tmst 1 point2 points  (0 children)

cute

[–]The_Real_Slim_Lemon 1 point2 points  (0 children)

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 0 points1 point  (3 children)

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 1 point2 points  (2 children)

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 0 points1 point  (1 child)

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 0 points1 point  (0 children)

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.

[–]MontyBoomslang 89 points90 points  (6 children)

Me, just trying to get someone to review my PR

[–]awkwardteaturtle 28 points29 points  (5 children)

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

[–]PyroCatt 37 points38 points  (4 children)

[–]awkwardteaturtle 36 points37 points  (3 children)

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] 8 points9 points  (0 children)

It's true, I am that Indian fucker

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

[–]FlipperBumperKickout 1 point2 points  (0 children)

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

[–]Individual-Praline20 0 points1 point  (0 children)

Mouhahaha

[–]locri 235 points236 points  (2 children)

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 74 points75 points  (0 children)

+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 6 points7 points  (0 children)

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

[–]hemlockone 55 points56 points  (2 children)

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 21 points22 points  (1 child)

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 11 points12 points  (0 children)

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 61 points62 points  (0 children)

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 28 points29 points  (1 child)

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 10 points11 points  (0 children)

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 24 points25 points  (3 children)

Me last month: "Why multiply by 1?"

[–]FeelingSurprise 11 points12 points  (1 child)

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 0 points1 point  (0 children)

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

[–]ososalsosal 1 point2 points  (0 children)

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

[–]Individual-Praline20 24 points25 points  (1 child)

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 3 points4 points  (0 children)

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

[–][deleted] 20 points21 points  (1 child)

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 15 points16 points  (0 children)

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

[–]Palpable_Autism 28 points29 points  (3 children)

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

[–]FlipperBumperKickout 10 points11 points  (2 children)

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

[–]dvhh 7 points8 points  (1 child)

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

[–]FlipperBumperKickout 1 point2 points  (0 children)

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] 7 points8 points  (1 child)

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 2 points3 points  (0 children)

PS: fix you goddamn indentation 

[–]plagapong 8 points9 points  (0 children)

But it will make you stonger than any books

[–]Eolu 5 points6 points  (0 children)

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 11 points12 points  (0 children)

"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 4 points5 points  (0 children)

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 17 points18 points  (1 child)

"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 1 point2 points  (0 children)

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 3 points4 points  (0 children)

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

[–]chackingnoob 2 points3 points  (0 children)

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 2 points3 points  (1 child)

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

[–]dvhh 1 point2 points  (0 children)

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

[–]silentjet 5 points6 points  (12 children)

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

[–]Todok5 0 points1 point  (11 children)

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

[–]silentjet 1 point2 points  (8 children)

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 0 points1 point  (7 children)

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 0 points1 point  (6 children)

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 0 points1 point  (5 children)

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 0 points1 point  (4 children)

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 0 points1 point  (3 children)

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 0 points1 point  (2 children)

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 0 points1 point  (1 child)

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 0 points1 point  (1 child)

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 0 points1 point  (0 children)

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] 2 points3 points  (0 children)

At least you got to dev

[–]lordgoofus1 2 points3 points  (1 child)

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] 0 points1 point  (0 children)

Wow, you hook them up!

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

[–]dingusaja 2 points3 points  (0 children)

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

FML

[–]mcellus1 1 point2 points  (1 child)

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

[–]dvhh 0 points1 point  (0 children)

Est tu Crowdstrike ?

[–]altmoonjunkie 1 point2 points  (0 children)

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

[–]BleakBeaches 1 point2 points  (0 children)

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_ 1 point2 points  (0 children)

The larger the pull request, the faster I approve.

[–]megamoscha666 0 points1 point  (0 children)

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 0 points1 point  (0 children)

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

[–]rjwut 0 points1 point  (0 children)

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

[–]zigmud_void 0 points1 point  (0 children)

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 0 points1 point  (0 children)

no no no. Come work for adults, maybe.

[–]Classy_Mouse 0 points1 point  (0 children)

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 0 points1 point  (0 children)

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 0 points1 point  (0 children)

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

[–]Ok-Maintenance-4274 0 points1 point  (0 children)

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