Comment by password4321 a day ago
Code complexity scanners⁰ eventually force pushing ifs down. The article recommends the opposite:
By pushing ifs up, you often end up centralizing control flow in a single function, which has a complex branching logic, but all the actual work is delegated to straight line subroutines.
⁰ https://docs.sonarsource.com/sonarqube-server/latest/user-gu...
One possibility is a file.py that is called by your framework. The interface could be something like
def doth_match(*args):
return True # the predicate
def doeth_thou(*args):
# processing things
return {} # status object for example
There could be multiple files rule1.py, rule2.py, etc that check and do different things.
This just moves decisions from inside of functions to the call site. At which point, there's more that can go wrong, since you're calling a function much more than it's single definition.
This is not obvious to me. The whole point was to separate the conditionals from the actions.
In your example, it’s not clear if/how much “should we do this” logic is in your function. If none, then great; you’ve implemented a find or lookup function and I agree those can be helpful.
If there’s some logic, eg you have to iterate through a set or query a database to find all the things that meet the criteria for “should do this”, then that’s different than what the original commenter was saying.
maybe: doThis( findAllMatchingThings( determineCriteriaForThingsToDoThisTo()))
would be a good separation of concerns
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.
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.
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.
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"™.
IME this is frequently a local optimum though. "Local" meaning, until some requirement changes or edge case is discovered, where some branching needs to happen outside of the loop. Then, if you've got branching both inside and outside the loop, it gets harder to reason about.
It can be case-dependent. Are you reasonably sure that the condition will only ever effect stuff inside the loop? Then sure, go ahead and put it there. If it's not hard to imagine requirements that would also branch outside of the loop, then it may be better to preemptively design it that way. The code may be more verbose, but frequently easier to follow, and hopefully less likely to turn into spaghetti later on.
(This is why I quit writing Haskell; it tends to make you feel like you want to write the most concise, "locally optimum" logic. But that doesn't express the intent behind the logic so much as the logic itself, and can lead to horrible unrolling of stuff when minor requirements change. At least, that was my experience.)
I've always hated code complexity scanners ever since I noticed them complaining about perfectly readable large functions. It's a lot more readable when you have the logic in one place, and you should only be trying to break it up when the details cause you to lose track of the big picture.
There was a thread yesterday about LLMs where somebody asked "what other unreliable tool people accept for coding?"
Well, now I have an answer...
tangent: how did you get that superscript 0 to render in an HN comment?
HN allows many Unicode characters, including U+2070 superscript zero which I copy+pasted after a web search. I'd actually be interested to learn the full allowlist.
https://en.wikipedia.org/wiki/Unicode_subscripts_and_supersc...
The way to solve this is to split decisions from execution and that’s a notion I got from our old pal Bertrand Meyer.
It complements or is part of functional core imperative shell. All those checks being separate makes them easy to test, and if you care about complexity you can break out a function per clause in the check.