Do all the things like ++ or -- rants, post your own rants, comment on others' rants and build your customized dev avatarSign Up
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.
AlmondSauce676648d4 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.
coder-guy23747dMark 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.
monkeyboy128547dIf 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
VaderNT152547d@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.
monkeyboy128546d@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.