It's really about finding the entry points into your program from the outside (including data you fetch from another service), and then massaging in such a way that you make as many guarantees as possible (preferably encoded into your types) by the time it reaches any core logic, especially the resource heavy parts.
manmal · 3m ago
It’s a bit niche for HN, but SwiftUI rendering works way better when following this. In a ForEach, you really shouldn’t have any branching, or you‘ll pay quite catastrophic performance penalties. I found out the hard way when rendering a massive chart with Swift Charts. All branching must be pushed upwards.
password4321 · 1h 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.
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 · 2m ago
Functions should decide or act, not both.
jt2190 · 46m 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 · 8m 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 · 4m 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.
daxfohl · 1h 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.)
ummonk · 23m 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.
marcosdumay · 23m 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...
neRok · 19m ago
I agree that the first example in the article is "bad"...
fn frobnicate(walrus: Option<Walrus>)`)
but the rest makes no sense to me!
// GOOD
frobnicate_batch(walruses)
// BAD
for walrus in walruses {
frobnicate(walrus)
}
It doesn't follow through with the "GOOD" example though...
fn frobnicate_batch(walruses)
for walrus in walruses { frobnicate(walrus) }
}
What did that achieve?
And the next example...
// GOOD
if condition {
for walrus in walruses { walrus.frobnicate() }
} else {
for walrus in walruses { walrus.transmogrify() }
}
// BAD
for walrus in walruses {
if condition { walrus.frobnicate() }
else { walrus.transmogrify() }
}
What good is that when...
walruses = get_5_closest_walruses()
// "GOOD"
if walruses.has_hungry() { feed_them_all() }
else { dont_feed_any() }
// "BAD"
for walrus in walruses {
if walrus.is_hungry() { feed() }
else { dont_feed() }
dcre · 1h ago
I took a version of this away from Sandi Metz’s 99 Bottles of OOP. It’s not really my style overall, but the point about moving logic forks up the call stack was very well taken when I was working on a codebase where we had added a ton of flags that got passed down through many layers.
Yeah, I immediately thought of "The Wrong Abstraction" by the same author. Putting the branch inside the for loop is an abstraction, saying "the for loop is the rule, and the branch is the behavior". But very often, some new requirement will break that abstraction, so you have to work around it, and the resulting code has an abstraction that only applies in some cases and doesn't in others, or you force a bunch of extra parameters into the abstraction so that it applies everywhere but is hard to follow. Whereas if you hadn't made the abstraction in the first place, the resulting code can be easier to modify and understand.
printInvoice(invoice, options) // is much better than
if(printerReady){
if(printerHasInk){
if(printerHasPaper){
if(invoiceFormatIsPortrait){
:
The same can be said of loops
printInvoices(invoices) // much better than
for(invoice of invoices){
printInvoice(invoice)
}
At the end, while code readability is extremely important, encapsulation is much more important, so mix both accordingly.
imcritic · 58m ago
This article doesn't explain the benefits of the suggested approach well enough.
And the last example looks like a poor advice and contradicts previous advice: there's rarely a global condition that is enough to check once at the top: the condition usually is inside the walrus. And why do for walrus in pack {walrus.throbnicate()} instead of making throbnicate a function accepting the whole pack?
No comments yet
daxfohl · 1h ago
I like this a lot. At first, putting ifs inside the fors makes things more concise. But it seems like there's always an edge case or requirement change that eventually requires an if outside the for too. Now you've got ifs on both sides of the for, and you've got to look in multiple places to see what's happening. Or worse, subsequent changes will require updating both places.
So yeah, I agree, pulling conditions up can often be better for long-term maintenance, even if initially it seems like it creates redundancy.
hinkley · 1h ago
There’s a lot of variable hoisting involved in loving conditional logic out of for loops and it generally tends to improve legibility. If a variable is loop invariant it makes debugging easier if you can prove it is and hoist it.
variadix · 14m ago
This is a great way of putting it.
The more things you can prove are invariant, the easier it is to reason about a piece of code, and doing the hoisting in the code itself rather than expecting the compiler to do it will make future human analysis easier when it needs to be updated or debugged.
jmull · 1h ago
I really don't think there is any general rule of thumb here.
You've really got to have certain contexts before thinking you ought to be pushing ifs up.
I mean generally, you should consider pushing an if up. But you should also consider pushing it down, and leaving it where it is. That is, you're thinking about whether you have a good structure for your code as you write it... aka programming.
I suppose you might say, push common/general/high-level things up, and push implementation details and low-level details down. It seems almost too obvious to say, but I guess it doesn't hurt to back up a little once in a while and think more broadly about your general approach. I guess the author is feeling that ifs are usually about a higher-level concern and loops about a lower-level concern? Maybe that's true? I just don't think it matters, though, because why wouldn't you think about any given if in terms of whether it specifically ought to move up or down?
hamandcheese · 25m ago
Isn't this just inversion of control? AKA the I in SOLID?
stevage · 1h ago
The author's main concern seems to be optimising performance critical code.
greesil · 54m ago
That's not clear to me. It first reads like "don't branch in a for loop (because parallelization?)" but I think it's more for keeping the code from becoming a mess over time with multiple developers.
hinkley · 1h ago
If your compiler is having trouble juggling a number of variables just think how much difficulty the human brain will have on the same code.
esafak · 1h ago
I agree, except for this example, where the author effectively (after a substitution) prefers the former:
fn f() -> E {
if condition {
E::Foo(x)
} else {
E::Bar(y)
}
}
fn g(e: E) {
match e {
E::Foo(x) => foo(x),
E::Bar(y) => bar(y)
}
}
The latter is not only more readable, but it is safer, because a match statement can ensure all possibilities are covered.
CraigJPerry · 52m ago
> Prefers the former after a substitution...
That's not quite right, it's a substitution AND ablation of 2 functions and an enum from the code base.
There's quite a reduction in complexity he's advocating for.
Further, the enum and the additional boilerplate is not adding type safety in this example. Presumably the parameters to foo and bar are enforced in all cases so the only difference between the two examples is the additional boilerplate of a 2-armed enum.
I strongly suspect in this case (but i haven't godbolted it to be sure) that both examples compile to the same machine code. If my hunch is correct, then the remaining question is, does introduction of double-entry book keeping on the if condition add safety for future changes?
Maybe. But at what cost? This is one of those scenarios where you bank the easy win of reduced complexity.
josephg · 45m ago
> The latter is not only more readable, but it is safer, because a match statement can ensure all possibilities are covered.
Whether or not this matters depends on what, exactly, is in those match arms. Sometimes there's some symmetry to the arms of an if statement. And in that case, being exhaustive is important. But there's plenty of times where I really just have a bit of bookkeeping to do, or an early return or something. And I only want to do it in certain cases. Eg if condition { break; } else { stuff(); }
Also, if-else is exhaustive already. Its still exhaustive even if you add more "else if" clauses, like if {} else if {} else {}.
Match makes sense when the arms of the conditional are more symmetrical. Or when you're dealing with an enum. Or when you want to avoid repeating conditions. (Eg match a.cmp(b) { Greater / Equal / Less } ).
The best way to structure your code in general really comes down to what you're trying to do. Sometimes if statements are cleaner. Sometimes match expressions. It just depends on the situation.
esafak · 22m ago
It's only exhaustive in this toy case. Add another one and what happens? The burden of exhaustive checking falls on your shoulders.
Mystery-Machine · 58m ago
Terrible advice. It's the exact opposite of "Tell, don't ask".
Performance of an if-statement and for-loop are negligent. That's not the bottleneck of your app. If you're building something that needs to be highly performant, sure. But that's not the majority.
It's really about finding the entry points into your program from the outside (including data you fetch from another service), and then massaging in such a way that you make as many guarantees as possible (preferably encoded into your types) by the time it reaches any core logic, especially the resource heavy parts.
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...
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.)
Well, now I have an answer...
And the next example...
What good is that when...https://sandimetz.com/99bottles
https://sandimetz.com/blog/2016/1/20/the-wrong-abstraction
And the last example looks like a poor advice and contradicts previous advice: there's rarely a global condition that is enough to check once at the top: the condition usually is inside the walrus. And why do for walrus in pack {walrus.throbnicate()} instead of making throbnicate a function accepting the whole pack?
No comments yet
So yeah, I agree, pulling conditions up can often be better for long-term maintenance, even if initially it seems like it creates redundancy.
The more things you can prove are invariant, the easier it is to reason about a piece of code, and doing the hoisting in the code itself rather than expecting the compiler to do it will make future human analysis easier when it needs to be updated or debugged.
You've really got to have certain contexts before thinking you ought to be pushing ifs up.
I mean generally, you should consider pushing an if up. But you should also consider pushing it down, and leaving it where it is. That is, you're thinking about whether you have a good structure for your code as you write it... aka programming.
I suppose you might say, push common/general/high-level things up, and push implementation details and low-level details down. It seems almost too obvious to say, but I guess it doesn't hurt to back up a little once in a while and think more broadly about your general approach. I guess the author is feeling that ifs are usually about a higher-level concern and loops about a lower-level concern? Maybe that's true? I just don't think it matters, though, because why wouldn't you think about any given if in terms of whether it specifically ought to move up or down?
That's not quite right, it's a substitution AND ablation of 2 functions and an enum from the code base.
There's quite a reduction in complexity he's advocating for.
Further, the enum and the additional boilerplate is not adding type safety in this example. Presumably the parameters to foo and bar are enforced in all cases so the only difference between the two examples is the additional boilerplate of a 2-armed enum.
I strongly suspect in this case (but i haven't godbolted it to be sure) that both examples compile to the same machine code. If my hunch is correct, then the remaining question is, does introduction of double-entry book keeping on the if condition add safety for future changes?
Maybe. But at what cost? This is one of those scenarios where you bank the easy win of reduced complexity.
Whether or not this matters depends on what, exactly, is in those match arms. Sometimes there's some symmetry to the arms of an if statement. And in that case, being exhaustive is important. But there's plenty of times where I really just have a bit of bookkeeping to do, or an early return or something. And I only want to do it in certain cases. Eg if condition { break; } else { stuff(); }
Also, if-else is exhaustive already. Its still exhaustive even if you add more "else if" clauses, like if {} else if {} else {}.
Match makes sense when the arms of the conditional are more symmetrical. Or when you're dealing with an enum. Or when you want to avoid repeating conditions. (Eg match a.cmp(b) { Greater / Equal / Less } ).
The best way to structure your code in general really comes down to what you're trying to do. Sometimes if statements are cleaner. Sometimes match expressions. It just depends on the situation.
Performance of an if-statement and for-loop are negligent. That's not the bottleneck of your app. If you're building something that needs to be highly performant, sure. But that's not the majority.
https://martinfowler.com/bliki/TellDontAsk.html