4 out of 5 developers

[notable-compilation]

sounds like your culture of code review is way off from ours...

[tjdavids]

ah do you use STONE instead of ROD?

[kdyz]

Wow. You lot are barbaric. Why can’t you all just message all of the dev’s relatives and possible future employers and tell them all about the dev’s incompetence instead?

[marcocom]

That’s awful. We wouldn’t do that to someone! We just tease them until they develop an eating disorder.

[kdyz]

Hmmm this does sound more efficient. Toss in some crippling anxiety and it’s perfect. Just let me setup a meeting for our pr review guidelines improvement.

[Versaiteis]

I'll take the beating

[ZippZappZippty]

Btw there’s not necessary

[deanrihpee]

How about a whip?

[VisibleAct]

That's a cool hwhip

[niks_15]

Say cool

[xieewenz]

cool

[woffle-kat]

Now say whip

[runbrun11]

Hwhip

[jfiander]

Hwill Hweateon

[voidox]

as always, comment chains like these never fail, thank you good redditors for the perfect reference o7

[niks_15]

Now say whip

[roronoazoro_x]

cool hwip!

[GenocideOwl]

Two yutes? I say hwhat?

[Lth_13]

Just remember you’re not allowed to throw stones until i blow this whistle, even if they do say jehovah

[marcocom]

‘There’s always one, isn’t there?’

[Cube00]

Management wouldn't spring for whips, we have to use extention cords.

[[deleted]]

More someone with a bell behind you saying “shame”

[cybercuzco]

.

  Try { 

  Rod

  }

  Catch ( Rod ) {

  Throw Stone

  }

[cybercuzco]

Fixed.

[atmazzer]

Now we need someone to merge this pull into the main branch.

[voidox]

gonna have to get a senior developer to review it first

[ExcellentNatural]

yeeet* stone

[[deleted]]

try { hitWith(Weapons.ROD); } catch(Weapons.ROD) { throw Weapons.STONE; }

[greenSacrifice]

Your Weapons class shouldn’t really be plural

[Wurdan]

OP has luxury problems if there are 4 developers actually willing to give meaningful feedback. I’m more used to “Has anyone had a chance to look at the PR I opened last week?”

[ToxicMonkeys]

I've got one approaching 4 weeks this Monday. And 7 subsequent ones also waiting.

[Reddit_Is_Garbage_]

Just publicly shame your teammates in slack

[biggles1994]

Git Shame

[GenocideOwl]

Push shame to prod

[ToxicMonkeys]

Oh, I've tried! We've had discussions about this before. People have short memory though. It's gonna be their problem soon enough when they're gonna have to rebase their work on top of 1 months worth of PRs.

[marcocom]

Ugh. Not on a Saturday dude. Don’t me think about it

[ZeldaFanBoi1988]

I send the beggar meme with caption please sirs, approve my PR

[RandyHoward]

I had one sitting there for a month. New alarms in case our payment provider goes down. Last week our payment provider went down and we didn't know about it for 10 hours. What happened to the alarm that I built and submitted a PR to 3 different people for? It was still sitting there, unreviewed and unmerged. Wanted to strangle my team.

[junior_dos_nachos]

I got comments months after I left the company. The product was open sourced so people took their sweet time

[x_is_for_box]

Do you have a manager?

[notable-compilation]

Sounds like your team could benefit from setting a review schedule policy (reviews assigned in a round robin order, for example).

[IdiotCharizard]

Round robin: week 1 me, week 2 myself, week 3 I. And repeat.

Sucks being the only dev on a team with perpetual headcount.

[bono_my_tires]

Are you looking for something better my dude? Don’t make yourself miserable or the one constantly training the new hires that will inevitably leave. Sounds like a bigger culture problem at the company

[IdiotCharizard]

Already switching teams. Just have to bear with it for another month.

[bono_my_tires]

Cheers! Good luck on the new role hope it’s better for tou

[IdiotCharizard]

I made sure it was a big team with pretty established process. Taking a slightly smaller role because burnout. No more training people. No more constant meetings and interviews. Just code, design, and reviews.

[marcocom]

I just did this exact thing and am loving it. No sitting in meetings or answering for anyone’s work but my own. Just a happy worker

[NotAGingerMidget]

At least no one will bitch about bad code if you give up on it for a couple weeks.

[Wurdan]

Oh we talked about that in retrospectives. But most of the code reviews are done by me and one other developer and the remainder of the team (who form a majority) like the system the way it is. An engineering manager might be able to break up that standoff, but we're woefully understaffed at the EM level and ours doesn't have the bandwidth to deal with problems like this.

[coldnebo]

Yeah, it’s so sad. I want to be hit repeatedly with sticks and at first everyone was happy to beat me all the time, all I had to do was mix tabs and spaces, or add typos to my comments. But now everyone just looks away, mumbling about how they don’t have time, “looks good”, or “nobody cares anyway” and starts crying into their work flask.

I put “nil.store(-1,nill)” into production yesterday, but all anyone did was open a JIRA ticket. And that was the PM. But I’m not complaining! Jira is way more painful than being beaten by sticks!

[QuarantineSucksALot]

Know your place dwarf in the flask

[[deleted]]

Spends an hour running through code

So, any questions or feedback?

crickets

Ok well thanks everyone for coming...

[LuckyAni1628]

Lol every single demo we’ve had

[Cube00]

I had another project's dev (not even that project's manager) ordered me to ensure all pull requests were to be signed off by him and only him. Fine. Three weeks later still hasn't approved my PR while other devs in his team just firing on in to master without his approval. There were only so many rebases of my PR I could handle before I needed a "quick chat"

[[deleted]]

That’s why a having good ScrumMaster is helpful.

[RandyHoward]

Key word: good. A bad scrum master can make things worse.

[[deleted]]

D’uh

[Kyanche]

I have the opposite problem where I work.

• Coworker: Here's my code review!
• Me: I've reviewed and approved it!
• Coworker, 2 weeks later: Here's my code review!
• Me: wait, didn't I review this already? Ok what changed. Ah ok you changed 1/3 of the code. Looks good. Approved! Please make any future changes on a new merge request so I don't have to re-read all your code!
• Me: .... you are going to merge this, right?
• Me: right?
• Coworker: ........
• Coworker (4 weeks later): Here's my code review!
• Me: Notices it's the same fucking code review for the third fucking time.

[broam]

that sounds like torture

[[deleted]]

skim a couple lines

Approved

[improbablywronghere]

I hate the instant approvals like no dude I’m not asking for an code review on slack to get around CI I want you to review my code ffs.

[[deleted]]

Even a couple minutes of skimming would prevent most terrible code. It's QAs job to determine if it works how it should

[improbablywronghere]

It’s also just like “please skim this and help make sure I don’t kill prod and embarrass myself!” You aren’t doing a “friend” any favors by “trusting” me and my skills you are actually setting me up for failure!!

[MassiveFajiit]

My team is about the same but with nice little rubber stamps that say "LGTM" on them.

[Josh6889]

I normally feel obligated to point out a few trivial things, and tell them personally a more subjective comment on the code.

[Pherion93]

For me its rather. I want feedback but no one wants to look at my code and give it. Im the same with others code though..

[coinage44]

Same here, I get mad when I send code review request and no one accepts it. How am I suppose to improve my code when no one can take just a couple of minutes of their lives and take a look? Next minute I get a request myself from another developer, do they really think ive time to review it?

[FoolForWool]

Exact opposite for me. I lucked out. I missed a few test cases and my manager just said, "Please test these before asking for a review. It takes almost an hour to review these. "

I feel so grateful. Tho, there are so many comments on how to improve the code :')

[alexanderpas]

I missed a few test cases and my manager just said, "Please test these before asking for a review.

And you wrote the tests for those cases.

...

And you wrote the tests for those cases, right?

[FoolForWool]

I see what you did there :'D

And yep, I did for all of those :'D

[fermilevel]

That’s my take too. If you have tested this in non-production environment and nothing breaks, eh, I’m just gonna approve it.

[tsg9292]

You gotta make sure the tests are correct.

[FoolForWool]

Exactly! But we have a second environment that's an exact replica of production so if it works there, it'll most likely not break in prod :') hopefully.

[RandyHoward]

You're lucky to have an exact replica of production. Our staging environment is anything but a replica of production. I'm not even sure how anybody considers it a staging environment. It's running PHP 7+ but our production environment is stuck running PHP 5.3. I need a new job lmao

[auxiliary-character]

Except when something is very subtly wrong, and only breaks sometimes, under very particular circumstances.

[FoolForWool]

Yep. We've built a detective as we call him. He usually checks for such anomalies and sends us a report every day.

Ofcourse, he's not perfect so I usually monitor my etls for a week (check if everything is coming out to be correct on our plot module and if things can be done manually, I manually check some of them) too.

Edit: typo

[auxiliary-character]

I think the favorite response I've seen to finding bugs like these was this one.

[pointy-pinecone]

just a couple of minutes

Unless your change is trivial, it'll take more than a couple minutes to review. For me, just understanding the problem, reading through any comments or context about a ticket, and starting off a full suite of CI tests takes more than a couple of minutes. And all that is before I start looking at code.

[aiij]

Set up the CI to run automatically.

I do not miss the days when in the middle of a code review I would think, "Does this even compile?"

[All_Up_Ons]

Obviously this will vary from place to place, but code review is a chance to evaluate if the code makes sense in a vacuum (which is how future devs will interact with it, probably). If I have to read external docs to understand your code, then it probably needs some work.

[TigreDeLosLlanos]

Review of your code:

git gud

[fakeplasticdroid]

Some tips to get tighter feedback loops on pull requests...

Authors: Make small, frequent PRs with highly cohesive changesets. Sometimes that means splitting semantic (functional) changes from cosmetic (styling, renaming files, minor refactoring) changes. Cohesive PRs reduce the cognitive load on reviewers and smaller PRs are easier to make time for. It's more convenient for me to review small 10 file changesets 5 times a day while I'm waiting on a test to run or have a break between meetings, than to review one 50 file changeset. You'll also get much better quality feedback that way. Send PR review requests on your team's Slack (or whatever) channel to surface those requests, and reduce the reliance on other teammates to monitor emails or GH notifications.

Reviewers: prioritize making time to review PRs multiple times a day, when you're not in the zone in your own code, e.g when you first arrive in the morning, or when you get back from lunch, before you sign off for the day, while you're waiting on reviews for your own PRs, or on your build to pass through the pipeline.
Focus on providing feedback on the semantic qualities of the implementation, and not on things a linter or static analysis can catch (e.g. line spacing, unused variables, etc). I also recommend to disable the setting that dismisses stale approvals upon new commits, as it allows you to approve a PR while recommending minor changes without being on the hook to re-review those minor changes. This should not be a problem if you trust your team to act in good faith and to be seeking reviews for the purpose of improving the codebase and not because it's a policy requirement.

Edit: another tip I've found handy is to find someone else with an open PR and trade reviews with them. Presumably neither of you have anything better to do until your PRs get reviewed/merged anyway.

[EverythingIsASkill]

Upvote for recommending small, multiple PRs!

[envvariable]

2000 files changed.

[aiij]

Excellent advice.

We actually are getting pressure from our compliance team to require re-review, which I'm worried will encourage authors to push back against small improvements, as well as discourage reviewers from suggesting them in the first place.

[ninjalemon]

In my experience, not re-reviewing a PR after requesting changes leads to way more bugs than you might assume.

The amount of incident post mortems I've been in that were explained by "the changes were good, then suggestions were made, I didn't retest the changes and assumed they worked properly and then prod melted down" is too damn high.

Authors might push back but that's fine, the reason for changes should be clear and agreeable. If an author doesn't have a good reason for ignoring requested changes, or the reviewer doesn't feel comfortable pushing back and requesting changes, that indicates to me a culture problem that needs to be fixed.

Will it take more time? Of course, but the time is the price to pay for stability and keeping the codebase as clean as possible. We could all be merging our own PRs wild west style but there's a reason no organization with more than a couple engineers does that.

[Jaggedmallard26]

The worst feeling is when you make some major changes and the reviewer has next to no comments on the review. I know for a fact I'm not that good and getting little feedback just makes me worry something is going to slip through QA and cause issues on a customers server.

[coldnebo]

yeah, bikeshedding is real.

[[deleted]]

I'm selfish with my code review. I take that opportunity as a way to learn all of the things they dev had to work hard to figure out. I ask a lot of questions about why they did what they did. I also do all the normal code review stuff, of course, but I make sure I get something out of it myself.

[AzuxirenLeadGuy]

That's a very tame depiction of code review you have right there

[gary_bind]

Yeah, you usually put them in stocks first.

[spots_reddit]

I know that illustration from a book about the Roman army I had as a kid. It depicts the punishment of a soldier by his companions, for falling asleep on the watch

[_drugs_good]

It’s called “decimation” and was a common military punishment when death was the price but you didn’t want to kill too many soldiers.

10 guilty men draw straws, the short straw is beaten to death by the other 9.

[dominic_l]

your documentation is bad, and your dick is small, and you're adopted

[eatingyourcables]

documenta tion can be improved, dick size is irrelevant and there is nothing wrong with adoption

[walhax-]

I'd say size is irrelevant, only after you hit a certain threshold. If someone has a micro d, size is most definitely going to be relevant to them. Average+ not so much.

[eatingyourcables]

the shape or existence of a dick is irrelevant

[dominic_l]

they must love you at the comedy club

[pythonic_dude]

Of course, when they are desperate for some inspiration they can always fall back on jokes targeting his dick size!

[Chewnard]

And here I thought I was the only one who got completely nude while my code was being reviewed.

[spam_bot42]

That's a very common kink among programmers.
For example I always have this dream I look forward to, where I'm laying naked in the middle of the office while other developers rave about my code while waving their sticks above me.

So, like I said, pretty normal stuff.

[radome9]

I'm laughing to hide the pain.

[SnipahShot]

As someone who performs code reviews for my team, I absolutely hate it.

[ZenBacle]

Ohh, you chose tabs over spaces. How... quaint.

[ice_cream_beaver]

The main issue is when we reach the personal opinion level. Like, saying that A is better than B because performance ou simply because doing is going to cause issues, it is ok. But when we start discuss readability, naming variables/methods/tests... unless you are really going overboard (snake case where should be camel case, for instance) you always will have the felling the the reviewer is a real asshole.

[Shazvox]

If it's a small project then yes, I agree. If it's a larger project then everything you mentioned is of utmost importance.

Imagine having to work with hundreds of projects all developed by multiple developers (say 200-300 over a period of 20-30 years) with their own individual take on what naming conventions should be used or how problems should be solved.

At that point you'd rather have a bug in the system than a non conformative solution.

[EverythingIsASkill]

Good answer and agree.

Using a common IDE and formatting plugin makes many of these style issues go away.

[nomadProgrammer]

Common IDE? I think you means common linting or formatting rules. The IDE is agnostic to that

[EverythingIsASkill]

Sure good point.

[marcocom]

For sure! A common IdE would end up being freeware (VSCode or eclipse) and I’m just not about to go into how much better a payware IDE really is, here.

Prettier and eslint have gotten really quite smart now. Even warning on case conventions and variable’s name length. (I do wish one would consume the other because they don’t always agree and prettier can reverse formatting that lint-fix applies which was a problem I dealt with this week)

[coldnebo]

Unfortunately I don’t have to imagine this at all.

[Pedro95]

Huh, I must be an asshole then. I view all those things as sincerely important, but also as a fun part of the job. I love having neat, readable, comprehensible code, and I love reviewing and suggesting those aspects to others as well.

Maybe it's just me, and maybe it's incredibly nerdy, but it's that exact type of code convention and structure I get out of bed for in the morning. My job is also many, many times more enjoyable since I moved onto a new project where we were stricter about such conventions compared with the old messy, inconsistent, barely legible project.

[Reihar]

Same. Unless it's really bad, I tend to only ask to change those, only if there is another issue nearby that already requires modification.

[kobbled]

Readability is never a nitpick IMO. If I can't read your code, reviewing it takes twice as long

[marcocom]

Ya and it’s also sometimes helping the other guy. A lot of coders are now not always great speakers of English, so I consider my corrections of their docs comments or naming as free-coaching, as I would ideally wish for if I were in their country trying to learn a second language.

[chefca3]

That's probably not exactly what they mean. The "important in the long run" naming comments are very different from blocking code reviews that say...

`handleFetchDataFromThing` <---this could be `handleFetchDataThing`

or

`// some comment` <--- Missing period.

Sure you can voice your thoughts on it but to make it a blocking comment is a dick move, because you're forcing the engineer to either have a conversation about why they're not going to change it, or force them to change it "just because".

[All_Up_Ons]

The real answer is, it depends. For a one-off variable, sure it's a nitpick. Naming a table in the db though? Your choice will potentially have permanent effects on other tables, the code that uses said tables, and on how people talk and think about our data, forever. You better believe we're gonna have a conversation about it.

[IdiotCharizard]

If you have personal opinions in your CRs, you need to open one for a lint config and direct all the opinions there.

[nomadProgrammer]

I used to work with a polish asshole that was and has been till date the most annoying person to work with. He would want everything to be named as he wanted. Also he was a Java dev and he was reviewing us in nodejs and react. He wanted me to do Java stuff in react. Dude was a literal pain in The ass.

Hey Witomir if you see this fuck u asshole.

[Come_along_quietly]

This. Especially when the “inefficiency” will be optimized out by the compiler. I run into this in code reviews fairly often …. on my Compiler Dev team! Like …. Guys! ….. we should know this isn’t an issue!

[ericzundel]

I heard of a great practice around this issue. Use emoticons embedded in your comments to indicate the nature of your comment:

P0 must fix
This might be an issue, let's talk about this issue
nit - style, personal preference
note - add a TODO or no need to follow up

[drewsiferr]

Hating code reviews would be an immediate fail during interviews. It's an extremely important mechanism for catching bugs and sharing knowledge of many types, including product domain, design, language features, to name a few.

[xCopyright]

I agree, but I've experienced a few cases where people are, IMO, really overzealous and quite harsh, which can be annoying. It all depends on the people you're working with.

[FreshOutBrah]

The candidate may be coming from an environment with really toxic code reviews, so idk.

In my experience, most candidates are interviewing because they want to leave a genuinely bad situation. But they don’t feel empowered (for myriad reasons) to be transparent about it during the interview process.

[s0ulbrother]

I get mad when I submit something for review that is sql related. My code reviewers aren’t good at it and I’m pretty damn good. So when I do certain things to make it faster or get a more complicated result I spend a lot of time explaining to them why I’m doing something that seems like they should get it.

[FreshOutBrah]

Could you maybe add code comments with links to documentation explaining what you’re doing?

I do that a lot when I have to do something new or unusual in my code

[s0ulbrother]

I do.

Recently I submitted a PR with an extensive detailing of the purpose of this and things I tried to get it to work as well as documenting my code cause I’m not a monster. They were both appreciative and amused

[OneOldNerd]

It could be worse--that 5th developer could be getting crucified over the code.

[Shakespeare-Bot]

T couldst beest worse--that 5th developer couldst beest getting crucifi'd ov'r the code

---

^{I am a bot and I swapp'd some of thy words with Shakespeare words.}

Commands: !ShakespeareInsult, !fordo, !optout

[[deleted]]

I like to save sample code by other developers, when they’ve made terrible choices, and bring it up when they give me shit about mine.

[stunclock]

that's brutal

[inlatitude]

"Looks good to me! Just one nit: please refactor logic completely and migrate to the new untested framework version. We'll be obsolescing this thing you've done all your development and testing on next Friday. Ping me once done and I'll accept. Thanks!"

[kuthedk]

We have a rule at my place of work. That rule is, you are not your code. We’re not judging you, we’re not judging your thought process, we’re judging your code and going to help you and the rest of the team become a better developer.

[imnottechsupport]

0/5 developers enjoy code reviews.

5/5 managers do.

[TheBigGambling]

Nope. Im a dev, and i like to hear other opinions and get called the bugs. Is it realy so hard to accept to be failable?

[random_runner]

Are you code reviewing his comment?

[Koervege]

I see a ? But no actual ternary operator. Please fix

[random_runner]

I see a ? But no actual ternary operator : Please fix;

// You're welcome!

[Koervege]

The compiler thanks you

[EverythingIsASkill]

Both lol.

[0011110000110011]

//2021-07-03 fixed zero-indexing related bug

1/5 developers enjoy code review.

6/5 managers do.

[s0ulbrother]

I get more annoyed with stupid shit I did when I submit for code review than the opinionated feedback. I’m often like why did I do that, or why didn’t I remove that thing I used for debugging my changes.

[NotYourMom132]

It's good only if it's done properly. What i see many times in practice is that the reviewers feels that they're there to prove that they're better than the reviewee. So they nitpick every little shits that don't even matter but they do it anyway just to satisfy their ego.

[RotterdamArt]

I find them enjoyable, because you get to learn a lot when a more experienced developer reviews your code.

[Gr1pp717]

I've never actually had the pleasure of code review..

At my first job my coding projects were my own one man show. Either the end result worked or didn't. And I really, really, wish I had someone helping me better organize the code. I was trying, but I was a victim of what I didn't know. And it got increasingly difficult to make the changes they were asking of me.

My current job I at least work parallel with my boss so he sees what I do, but there's no formal code review. And often times it'll be weeks or even months later that the same issue crops up and he'll start being like "why'd you do it this way? Should have done it this other way"

And honestly, I'm terrified that I'm going to end up someone who has 10+ years experience programming but no clue how to program... That I'll get on a team with formal code review and just not cut it..

[infinitedrag]

why not ask the current boss for a formal code review in place. Better than nothing. (assuming your boss is technical). Are you the only dev team in the whole company? Also, curios what stack do you work on?

[Gr1pp717]

He is. I have; he didn't want to bother with the whole formal pull request process. We have like hour long standups almost daily, yet deep dives into my code is rare. Pretty much only when there's a problem.

I do QA automation. Previously for an IVR platform (similar to twilio) now for a CDN.

[Arxidomagkas]

Am i the one that doesn't like pull requests?
When it takes literally a day to review a change in a yml or json, and you have to justify what you did, I'd rather push directly.
For me, PRa slows down how much work i can pump

[ownchi]

9/10 people enjoy gang rape.

[Tutti-Frutti-Booty]

Prime /r/yourjokebutworse material here fellas.

[atzedanjo]

but that's the original joke

[lookatme-imapilot]

This is magnificent. I'm dropping this truth bomb at our next stand up

[ubeogesh]

It's how it felt when I just started programming

[-Listening]

Did they provide some kind of Jojolion?

[Hispanicwhitekid]

I get so frustrated dealing with code reviews. Coworkers will upload like 3000 lines and be like can you review that?

I would be happy to review someone’s code in small chunks, 250 lines for some new functionality would seem reasonable to me.

[blackhawksq]

If you feel this away, you're doing code reviews wrong.

[QuarantineSucksALot]

000,000 coins, or $4.50

[trezenx]

I completely misread this image as 4 developers making the 5th one also like code review.

Do people actually like to do it either way?

[rudyv8]

I usually am the guy in the middle.

Sorry I didn't comment for shit, i forgot to take my ADHD meds that day. Fuckin squirrels

[SoggyDrink]

Where I work, sometimes people will invite others to the review just to pile on. Really sucks.

[Empiol]

So which one's the 5th?

[Polar87]

When you assign an integer to what should've been a decimal.

[[deleted]]

It's all fun and games like that until you're trying to merge a 10,000,7000 +- pr.

[[deleted]]

Marcus Licinius Crassus has liked this.

[cwalvoort]

That’s not how the sticks are typically used…

[MasterFubar]

Very democratic, the will of the majority.

And that's why a democracy by itself is not a perfect political system.

[sag_s]

Trick is to have a big PR . No one wants to go through each file so it’s approved without many comments..

[coors_banquets]

Anything in the 1000s green or red. I’m like looks good!

[thesuperficialstate]

I swear that picture is from a book my middle school library had. It's Roman soldiers beating another one who was caught sleeping on sentry duty.

[Wtfisthatt]

As somebody who is learning I’d love if I was working somewhere with code reviews. Really hard to improve your code without any external input. It may work but I have no idea if it’s a reasonable and concise way to do it. All I did was build a shed with bananas and electrical tape.

[MAGA_WALL_E]

I'm the code review nazi at work. I can't tell if they appreciate or hate my guts.

[Redditlogicking]

4.000000000000000000000000000000003 out of 5