undefined | Better HN

0 points_kidlike9d ago0 comments

I'm very surprised by these comments...

I regularly review code that is way more complicated that it should.

The last few days I was going back and forth on reviews on a function that had originally cyclomatic complexity of 23. Eventually I got it down to 8, but I had to call him into a pair programming session and show him how the complexity could be reduced.

0 comments

servo_sausage9d ago

Someone giving work like that should be either junior enough that there is potential for training them, so your time investment is worth it, or managed out.

Or it didn't really matter that the function was complex if the structure of what's surrounding it was robust and testable; just let it be a refactor or bug ticket later.

_kidlikeOP8d ago

he is a junior yes.

zzrrt8d ago

I know the aggravation of getting a hairball of code to review, but I often hold my nose. At least find a better reason to send it back, like a specific bug.

If you're sure cyclomatic complexity should be minimized, I think you should put such rules in a pre-commit hook or something that runs before a reviewer ever sees the code. You should only have to help with that if someone can't figure out how to make it pass.

If you're not willing or politically able to implement that, you might be wasting time on your personal taste that the team doesn't agree with. Personally I'm pretty skeptical of cyclomatic complexity's usefulness as a metric.

_kidlikeOP8d ago

I just used it here to approximately convey the scale.

the original function was full of mutable state (not required), full of special cases (not required), full of extra return statements (not required). Also had some private helper methods that were mocked in the tests (!!!).

All of this just for a "pure" function. Just immutable object in - immutable object out.

and yes, he was a junior.

surajrmal8d ago

I always approve a change with comments for nits that are optional to address. I only hold back approval if there is a legitimate flaw of some sort. Generally this leads to small changes almost always getting approved on the first shot, but larger changes needing at least one back and forth. AI code review tools make it much easier to spot legitimate problems these days.

j / k navigate · click thread line to collapse

0 comments

servo_sausage9d ago

Someone giving work like that should be either junior enough that there is potential for training them, so your time investment is worth it, or managed out.

Or it didn't really matter that the function was complex if the structure of what's surrounding it was robust and testable; just let it be a refactor or bug ticket later.

_kidlikeOP8d ago

he is a junior yes.

zzrrt8d ago

I know the aggravation of getting a hairball of code to review, but I often hold my nose. At least find a better reason to send it back, like a specific bug.

_kidlikeOP8d ago

I just used it here to approximately convey the scale.

All of this just for a "pure" function. Just immutable object in - immutable object out.

and yes, he was a junior.

surajrmal8d ago

j / k navigate · click thread line to collapse