Live data from Hacker News

Push Ifs Up and Fors Down

matklad.github.io

31–40 of 209 posts

Re: Push Ifs Up and Fors Down

#31
post #11
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.

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.

Re: Push Ifs Up and Fors Down

#32
post #9

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

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.

[deleted]

Re: Push Ifs Up and Fors Down

#33
post #19

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.

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.

Re: Push Ifs Up and Fors Down

#34
post #33
post #19

Earlier quoted context omitted.

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…

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.

Re: Push Ifs Up and Fors Down

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

Re: Push Ifs Up and Fors Down

#36
post #9

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

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.

Functions should decide or act, not both.

Re: Push Ifs Up and Fors Down

#37
post #10
post #7

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. https://sandimetz.com/99bottles

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 ex…

I can recommend this article about the "midlayer mistake" https://lwn.net/Articles/336262/

Re: Push Ifs Up and Fors Down

#38
The example listed as “dissolving enum refactor” is essentially polymorphism, i.e. you could replace the match by a polymorphic method invocation on the enum. Its purpose is to decouple the point where a case distinction is established (the initial if) from the point where it is acted upon (the invocation of foo/bar). The case distinction is carried by the object (enum value in this case) or closure and need not to be reiterated at the point of invocation (if the match were replaced by polymorphic dispatch). That means that if the case distinction changes, only the point where it is established needs to be changed, not the points where the distinct actions based on it are triggered.

This is a trade-off: It can be beneficial to see the individual cases to be considered at the points where the actions are triggered, at the cost of having an additional code-level dependency on the list of individual cases.

Re: Push Ifs Up and Fors Down

#39
post #9

Earlier quoted context omitted.

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.

Functions should decide or act, not both.

This is an excellent rule.

Re: Push Ifs Up and Fors Down

#40
post #30

I agree that the first example in the article is "bad"... fn frobnicate(walrus: Option )`) 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…

I think the "push for loops down" is missing a bit of detail about the why. The author alludes to "superior performance" but I don't think makes it clear how that can happen.

Vectorization is a bit obscure and a lot of coders aren't worried about whether their code vectorizes, but there's a much more common example that I have seen shred the performance of a lot of real-world code bases and HTTP APIs, which is functions (including APIs) that take only a single thing when they should take the full list.

Suppose we have posts in a database, like for a forum or something. Consider the difference between:

    posts = {}
    for id in postIDs:
        post[id] = fetchPost(id)
versus

    posts = fetchPosts(postIDs)
fetchPost and fetchPosts both involve hitting the database. The singular version means that the resulting SQL will, by necessity, only have the one ID in it, and as a result, a full query will be made per post. This is a problem because it's pretty likely here that fetching a post is a very fast (indexed) operation, so the per-query overhead is going to hit you hard.

The plural "fetchPosts", on the other hand, has all the information necessary to query the DB in one shot for all the posts, which is going to be much faster. An architecture based on fetching one post at a time is intrinsically less performant in this case.

This opens up even more in the HTTP API world, where a single query is generally of even higher overhead than a DB query. I think the most frequent mistake I see in HTTP API design (at least, ignoring quibbling about which method and error code scheme to use) is providing APIs that operate on one thing at a time when the problem domain naturally lends itself to operating on arrays (or map/objects/dicts) at a time. It's probably a non-trivial part of the reason why so many web sites and apps are so much slower than they need to be.

I find it is often easy to surprise other devs with how fast your system works. This is one of my "secrets" (please steal it!); you make sure you avoid as many "per-thing" penalties as possible by keeping sets of things together as long as possible. The "per-thing" penalties can really sneak up on you. Like nested for loops, they can easily start stacking up on you if you're not careful, as the inability to fetch all the posts at once further cascades in to you then, say, fetching user avatars one-by-one in some other loop, and then a series of other individual queries. Best part is, profiling may make it look like the problem is the DB because "the DB is taking a long time to serve this" because profiles are not always that good at turning up that your problem is per-item overhead rather than the amount of real work being done.

Post reply on HN