Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Push Ifs Up and Fors Down

matklad.github.io

Post reply on HN
Print view View original

11–20 of 209 posts

Re: Push Ifs Up and Fors Down

#11

Post by hinkley »

daxfohl wrote: post #5

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…

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.

Re: Push Ifs Up and Fors Down

#14

Post by Mystery-Machine »

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.

https://martinfowler.com/bliki/TellDontAsk.html

Re: Push Ifs Up and Fors Down

#15

Post by imcritic »

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?

Re: Push Ifs Up and Fors Down

#16

Post by Kuyawa »

Push everything down for better code readability

  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.

Re: Push Ifs Up and Fors Down

#17

Post by greesil »

stevage wrote: post #4

The author's main concern seems to be optimising performance critical code.

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.

Re: Push Ifs Up and Fors Down

#18

Post by CraigJPerry »

esafak wrote: post #2

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.

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

Re: Push Ifs Up and Fors Down

#19

Post by jt2190 »

password4321 wrote: post #3

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

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.

Re: Push Ifs Up and Fors Down

#20

Post by josephg »

esafak wrote: post #2

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.

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

Post reply on HN