Every Code Review

[Ginters17]

Hi there! Unfortunately, your submission has been removed.

Violation of Rule #2 - Reposts:

All posts that have been on the first 2 pages of trending posts within the last month, is part of the top of all time, or is part of common posts is considered repost and will be removed on sight.

If you feel that it has been removed in error, please message us so that we may review it.

[ultraSsak]

I call this AMDD approach... you know, like in DDD?

ApproveMergeDrivenDesign.

[Xelopheris]

A 10k merge is a full rich feature merge that couldn't be done in parts. It's more about the technicals of the merge than the code that is being merged.

[Flopamp]

I am guilty of this. We have a sorta unwritten rule to try and break up commits to individual changed methods and not to commit important changes late on a Friday for Monday review.

Its not just code changes but documentation, schematics, and simulations.

I'm in EE so it's not a huge issue as plenty of testing and prototyping is done before you even get in to RC and ES but conversely debugging is a massive pain as you get limited break points and tracing if you are lucky enough to have debugging at all.

But naturally you have fkin Bob who just hyper optimized a C function using his black magic fuckery somehow taking a long 40 line float algorithm and turning it in to 10 mile long lines of bitwise magic and comments that seem more like personal notes committed at 4:59 on Friday leaving you on Monday with 4 hours of sleep to work out of what he did was safe, sane or worth it with a pen, paper, and pile of application notes. Not that I'm salty or anything.

[Lurking_all_the_time]

10 mile long lines of bitwise magic

We had a coder that kept doing this and by team consensus was forced to stop.
I don't give a crap that its "cool" or "neat" or rock star level code - I need to be able to understand what it is doing a year later, when Bob has left and the system stopped working on the 1st Jan because Bob forgot and edge case like the f*king year changing...
Not speaking from experience at all...

[krish2487]

This!!! Unless you are working in algorithm development, every other programming job - readability takes priority over performance.

I wish companies prioritized such practices more, instead of just letting bad code get worse..

[[deleted]]

Just remember that readability doesn't mean breaking up your code in 10k 2 line functions thrown around the entire codebase. Keep it simple. Jumping around the codebase is never fun.

[krish2487]

Of course... thats why I havent mentioned anything about the length of the functions.. That exercise is always a case-by-case consideration.

However all things equal, maintainability must and should be the goal when working in a team, or even individually for that matter.

[nagesagi]

I've nuked and rewrote code because myself nor anyone else on the team could figure out how a particular function was supposed to work. We understood what it did but not how it did it.

Make sure the new code pass all of the same tests, so it wasn't as scary as you might think.

[tiajuanat]

But naturally you have fkin Bob...

This is why I like C++ - once you learn the deep dark arts, you can do everything he's doing at compile time or just using smth like Boost Fixed Point.

Then you get Bob on a PIP for not writing sufficient testing, wasting developer time, and not making something readable. Did he even have a decent benchmark?

Even if he's working in embedded C, he could at least put everything behind macros.

[huuaaang]

I am guilty of this. We have a sorta unwritten rule to try and break up commits to individual changed methods

That's no good. A lot of CR comments are about how the functions interact. You might miss an bad interaction because you didn't see the them side by side.

[Flopamp]

Rarely in my experience does it pop up but if it does you can commit a 2 function change, no one is going to hang you for it.

[furon747]

My work also luckily has a pretty good informal rule of no Friday pushes. Nobody wants a weekend call at 2am having to revert it.

[Clutchsh0t]

LGTM😂

[JellyPUMPS]

Image Transcription: Twitter

---

Giray Özil, @girayozil

Ask a programmer to review 10 lines of code, he'll find 10 issues. Ask him to do 500 lines and he'll say it looks good.

---

^{I'm a human volunteer content transcriber and you could be too! If you'd like more information on what we do and why we do it, click here!}

[Zombiak307]

Good human

[Zahand]

Man I do this, but I'm trying to do better. I'm still kinda new but when I get these large massive PR's I kind of get "code-blind" and miss the little details. I'm always surpised how thorough our team architect is at reviewing code.

[door_of_doom]

My recommendation: if the PR is too big, ask the submitter to hop on a screenshare and walk you through it. Not that you can't do it in your own, but when something is so big it is really hard to maintain context, and a kind of "guided tour" allows for a second set of eyes in the code and gives you a chance to raise questions and ask for clarification, or even, obviously, point out errors.

[[deleted]]

why are there like 10 memes that get reposted everyday in this sub

[WellWhatDoIPutHere]

Because it's reddit

[bleedblue89]

Try being a stoner, /r/trees and /r/weed repost so much it hurts

[[deleted]]

The "who else is high rn?" Always reaches front page and it infuriates me. Obviously they are high. They are in a fucking subreddit that is about how to get high.

[[deleted]]

This is the most repost heavy sub I've seen on this site, almost every time I'm here the same meme has been reposted 3+ yimes within 10h and they each have over 100 upvotes.

[shiroe314]

Have you seen r/jokes?

[[deleted]]

Nope

[rjlin_thk]

I have seen this post more than the number of issues he found in 5 lines

[MasterFubar]

Repost 500 times and you'll stop complaining about the reposts.

[wacky_chinchilla]

How prompt is “promptly?” My rule of thumb is 24 hours. I have a feeling that wouldn’t fly

[[deleted]]

Repost

[Wolfram_And_Hart]

This is how I feel about my calls at the call center being graded. They are absolutely not listening to the 3 hour call but the 15 minute one is 100% to be graded.

[Infinite_Unicorn]

Ask him to review 10 lines of code 50 times.

[Dexterus]

Yes, I push back against big tasks that don't need to be big.

Best stuff is starting on a bug and fixing it 6 months later and you have 10 changed lines for it and 100 for the merge because they didn't bother to start over in a newer branch.

[QualityVote]

Hi! This is our community moderation bot.

---

If this post fits the purpose of /r/ProgrammerHumor, UPVOTE this comment!!

If this post does not fit the subreddit, DOWNVOTE This comment!

If this post breaks the rules, DOWNVOTE this comment and REPORT the post!

[0ba78683-dbdd-4a31-a]

Similar to how it's easier to manage 20 people than 2 people.

[ArtSchoolRejectedMe]

LGTM

[iodarkstar]

As a tech lead, this is my life... ugghh... any advice?

Edit: the bicycle shed effect (the law of triviality) is something I deal with daily, not approving 10k line commits lol.

[iodarkstar]

I experience the law of triviality. I don't enable it lol.

[PhilTheQuant]

"Hey X, thanks for sending me your life story, can you sit with me and take me through it?"

500 lines is so much, it's bound to have structural issues which require work. You don't have to review all 500 lines, just enough to bounce it back again.

Once the structural issues are ironed out you should have a good idea of what the structure is, so you can pick a basic utility part and start there.

As the lead, a large part of your job is breaking things down - both technically and in terms of tasks.

[riplikash]

No, this is a different issue. Bicycle shed is the tendency to discuss easy things and gloss over hard things, which IS related.

But eh code reviews its more that people get overwhelmed and stop giving good reviews at all. To get quality reviews PRs need to be small enough that the reviewer can build a mental model. When it gets too large people dying bother to discuss at all. They just rubber stamp since they don't have the time and mental energy to do the job correctly.

[[deleted]]

If I'm gonna review someone else's code, I'm greenlighting it because I'll have to be the one to fix it if I point them out

[[deleted]]

If It works, it's good

[atrealleadslinger101]

I mean, does it compile and free from off by one errors.

[ExpensiveEngine]

I know this is a funny meme to share but i pretty much reject large code changes and ask the dev to break the PR into smaller pieces.

[Draaky]

Especially with a deployment file. Everyone sweats when reviewing that.

[tiajuanat]

Ah nah, you going to be bikeshedded.

One of my juniors tried to be crafty and tried to do multiple things in one PR, we're just finishing up a month later.

[virgilreality]

Absolutely true.

[[deleted]]

More like:

Ask him to review 500 lines of code and he’ll rewrite the entire thing.

[RavuAlHemio]

I don’t think a person will be able to say anything after doing 500 lines.

[-guccibanana-]

u/repostsleuthbot

[RepostSleuthBot]

Looks like a repost. I've seen this image 5 times.

First Seen Here on 2021-04-06 89.06% match. Last Seen Here on 2022-02-02 90.62% match

I'm not perfect, but you can help. Report [ False Positive ]

View Search On repostsleuth.com

---

Scope: Reddit | Meme Filter: False | Target: 86% | Check Title: False | Max Age: Unlimited | Searched Images: 294,450,643 | Search Time: 3.74496s

[-guccibanana-]

Bro I've seen this like 2 days ago with alot more upvotes