Comment by jt2190 a day ago
Code scanners reports should be treated with suspicion, not accepted as gospel. Sonar in particular will report “code smells” which aren’t actually bugs. Addressing these “not a bug” issues actually increases the risk of introducing a new error from zero to greater than zero, and can waste developer time addressing actual production issues.
Cyclomatic complexity may be a helpful warning to detect really big functions, but the people who worry about cyclomatic complexity also seem to be the sort of people who want to set the limit really low and get fiesty if a function has much more than a for loop with a single if clause in it. These settings produce those code bases where no function anywhere actually does anything, it just dispatches to three other functions that also don't hardly do anything, making it very hard to figure out what is going on, and that is not a good design.
I call this "poltergeist code". Dozens of tiny functions that together clearly does something complex correctly, but it's very hard to find where and how it's actually done.
One incomplete but easy to state counter "rule" to fling back in such cases is just: If the function isn't generic and re-used for other unrelated things, then it probably shouldn't be a seperate function.
Yeah only probably, there can sure be large distinct sub-tasks that aren't used by any other function yet would improve understanding to encapsulate and replace with a single function call. You decide which by asking which way makes the overall ultimate intent clearer.
Which way is a closer match to the email where the boss or customer described the business logic they wanted? Did they say "make it look at the 3rd word to see if it has trailing spaces..."?
Or to find out which side of the fuzzy line a given situation is falling, just make them answer a question like, what is the purpose of this function? Is it to do the thing it's named after? Or is it to do some meaningless microscopic string manipulation or single math op? Why in the world do you want to give a name and identity to a single if() or memcpy() etc?
I get that, and I think it’s a fine check for junior devs especially. In my experience, mostly with PHP mess detector, Cyclomatic Complexity smells typically manifest as nested conditionals. I’m at a point now where I try to keep that minimal myself, but I have peers that often have these diving sets of if statements that irk me. I’d rather invert a conditional and return early if needed, or at least separate the logic so that there’s maybe two levels at most.
I do agree that, in general, a senior engineer should be able to suss out what’s too complex. But if the bar is somewhat high and it keeps me from sending a PR back just for complexity, that’s fine.
The tools are usually required for compliance of some sort.
Fiddling with the default rules is a baby & bathwater opportunity similar to code formatters, best to advocate for a change to the shipping defaults but "ain't nobody got time for that"™.
I agree with you. Cyclomatic complexity check may be my least favorite of these rules. I think any senior developer almost always “knows better” than the tool does what is a function of perfectly fine complexity vs too much. But I have to grudgingly grant that they have some use since if the devs in question routinely churn out 100-line functions that do 1,000 things, the CCC will basically coincidentally trigger and force a refactor which may help to fix that problem.