How do you guys deal with reviewing garbage code?

Do you waste a ton of time, trying to understand the code, so you can write constructive feedback?

Today I wasted 4 hours reviewing something like that and I'm still not finished.

You'll need to provide some examples.

If it's long enough to take 4 hours, you'll probably want to ask them to break it up into consumable chunks, and have discussions around sub-tasking and feature flags.

4 hours and still going, and it's garbage?! Something's wrong there.

If it's a massive feature with silly amounts of code, then it needs to be checked way more frequently, in small increments. Review separate sub-features onto the main feature branch in that case.

If it's a short piece of code but just genuinely that terrible, then I'd seriously make the argument that it needs to fundamentally be re-architected / rewritten rather than going over every last piece. If that's the case, get them to do it in stages while you check on it to avoid more work wasted. If it *keeps* happening, then it sounds like someone needs some training.

On the other hand, if it's just a specialist domain (ASM, CUDA, even OpenGL sometimes etc.) then it may well just take that long to go through it thoroughly since it's not a high level language / obvious algorithm / something that can easily fit into a paradigm - and if that's the case, I'd just go with it. Shouldn't happen often though.

Mark as needs improvement and have a nice face-to-face. Don't waste any more time.

Sorry for not prodviding more context.

I mainly have a problem with a single class, with a very simple concept, like @AlmondSauce said in 3rd paragraph.

I guess deep down I already know what to do – talk it out. It's just a bit hard to find the right words to not seem like someone who likes to swing around his di*k.

Thanks for answers.

@WildOrangutan Just think about phrasing, approach it as a "how can we talk about improving this code together because idiots like me can't make sense of it!" and you'll be fine. You're one of the better guys just for not wanting to seem like a dick.

If I am unfamiliar with the area of code then I scan it quickly for obvious pitfalls. Sometimes that means the review lasts less than a minute. If I am familiar with the area or have concerns about what's being done, I will fetch the branch and run the code as well as look at the code. Max time for that would be about an hour. Spending more time if governed by the law of diminishing returns. My opinion: software reviews are mostly a waste of time. Sure, they do catch some things, but don't spend a lot of time on them.

I take a sip, sit next to the guy responsible and walk him through everything and allow him to explain it in his own words

@WildOrangutan I often feel like that as well. I've adopted the approach of asking questions to understand or challenge the authors thinking. Why did they choose that approach or did they consider other ways.

Luckily I have to do that very rarely

@monkeyboy "My opinion: software reviews are mostly a waste of time. Sure, they do catch some things, but don't spend a lot of time on them."

A lot can be caught through code review. Bugs, inconsistencies to other code, bad understandability. The other aspect you're missing is getting familiar with code and the features it implements.

In my opinion, if the reviewer didn't find at least one thing to improve, they didn't do their job properly.

@VaderNT I disagree (and that's OK). I can be a little more specific by rephrasing my opinion to "code reviews are mostly a waste of time compared to other things you could be doing". Plus, the whole attitude about needing to find something in a code review is a colossal waste. I will never ask for a change related to style or convention or anything else that is not functional (OK, there's bound to be an exception or two but not many). Why? Because it doesn't matter. If I wrap up a complicated set of changes and spend 3 days shepherding it through a maze of Jira hoops and countless build failures due to unrelated tests only to have a reviewer tell me I should expression based properties because they are "newer". At that point, you're just taking the company's money and setting it on fire. If it runs and is reasonable, move on.