7 ms·

Code complexity scanners⁰ eventually force pushing ifs down. The article recommends the opposite: By pushing ifs up, you often end up centralizing control flow

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-guide/code-metrics/metrics-definition/#cyclomatic-complexity https://docs.sonarsource.com/sonarqube-server/latest/user-gu...

daxfohl 1y ago
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.)
hinkley 1y ago
The way to solve this is to split decisions from execution and that’s a notion I got from our old pal Bertrand Meyer. if (weShouldDoThis()) { doThis(); } 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.
- 0cf8612b2e1e 1y ago
  Functions should decide or act, not both.
  - d_burfoot 1y ago
    This is an excellent rule.
  - swat535 1y ago
    But if that’s all you have, then how does your system do anything ? You ultimately need to be able to decide and then act based in that decision somewhere..
    - turtleyacht 1y ago
      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 The framework loops and checks the first function; if true, then execute the second function. And then break or continue for other rule files (or objects). There could be multiple files rule1.py, rule2.py, etc that check and do different things.
      - gameman144 1y ago
        I think the parent's argument is that wherever in your framework you're calling `doth_match` and then `doeth_thou`, you have a single function that's both deciding and acting. There has to be a function in your program that's responsible for doing both.
        hinkley 1y ago
        A function that asks a question with one function call and calls another based on the answer isn’t doing any work or asking any questions. It’s just glue code. And as long as it stays just glue code, that’s barely any of your code. Asking for absolutes is something journeymen developers need to grow out of. The principle of the excluded middle applies to Boolean logic and bits of set theory and belongs basically nowhere else in software development. But it’s a one trick pony that many like to ride into the ground.
  - 1y ago
- deleted 1y ago
  [deleted]
- btown 1y ago
  To add to this, a pattern that's really helpful here is: findThingWeShouldDoThisTo can both satisfy a condition and greatly simplify doThis if you can pass it the thing in question. It's read-only, testable, and readable. Highly recommend.
  - efitz 1y ago
    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
    - hinkley 1y ago
      let list = findHumansToKill(); //list = []; killHumans(list);
      - deleted 1y ago
        [deleted]
jt2190 1y 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.
- xp84 1y ago
  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.
  - jerf 1y ago
    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.
    - BurningFrog 1y ago
      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.
      - Brian_K_White 1y ago
        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?
deleted 1y ago
[deleted]
marcosdumay 1y ago
There was a thread yesterday about LLMs where somebody asked "what other unreliable tool people accept for coding?" Well, now I have an answer...
ummonk 1y ago
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.
chrisweekly 1y ago
tangent: how did you get that superscript 0 to render in an HN comment?
- password4321 1y ago
  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_superscripts#Superscripts_and_subscripts_block https://en.wikipedia.org/wiki/Unicode_subscripts_and_supersc...

daxfohl 1y ago

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.)

hinkley 1y ago

The way to solve this is to split decisions from execution and that’s a notion I got from our old pal Bertrand Meyer. if (weShouldDoThis()) { doThis(); } 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.

0cf8612b2e1e 1y ago
Functions should decide or act, not both.
- d_burfoot 1y ago
  This is an excellent rule.
- swat535 1y ago
  But if that’s all you have, then how does your system do anything ? You ultimately need to be able to decide and then act based in that decision somewhere..
  - turtleyacht 1y ago
    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 The framework loops and checks the first function; if true, then execute the second function. And then break or continue for other rule files (or objects). There could be multiple files rule1.py, rule2.py, etc that check and do different things.
    - gameman144 1y ago
      I think the parent's argument is that wherever in your framework you're calling `doth_match` and then `doeth_thou`, you have a single function that's both deciding and acting. There has to be a function in your program that's responsible for doing both.
      - hinkley 1y ago
        A function that asks a question with one function call and calls another based on the answer isn’t doing any work or asking any questions. It’s just glue code. And as long as it stays just glue code, that’s barely any of your code. Asking for absolutes is something journeymen developers need to grow out of. The principle of the excluded middle applies to Boolean logic and bits of set theory and belongs basically nowhere else in software development. But it’s a one trick pony that many like to ride into the ground.
- 1y ago
deleted 1y ago
[deleted]
btown 1y ago
To add to this, a pattern that's really helpful here is: findThingWeShouldDoThisTo can both satisfy a condition and greatly simplify doThis if you can pass it the thing in question. It's read-only, testable, and readable. Highly recommend.
- efitz 1y ago
  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
  - hinkley 1y ago
    let list = findHumansToKill(); //list = []; killHumans(list);
    - deleted 1y ago
      [deleted]

jt2190 1y 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.

xp84 1y ago
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.
- jerf 1y ago
  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.
  - BurningFrog 1y ago
    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.
    - Brian_K_White 1y ago
      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?

deleted 1y ago

[deleted]

marcosdumay 1y ago

There was a thread yesterday about LLMs where somebody asked "what other unreliable tool people accept for coding?" Well, now I have an answer...

ummonk 1y ago

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.

chrisweekly 1y ago

tangent: how did you get that superscript 0 to render in an HN comment?

password4321 1y ago
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_superscripts#Superscripts_and_subscripts_block https://en.wikipedia.org/wiki/Unicode_subscripts_and_supersc...