Push Ifs Up and Fors Down
matklad.github.io
matklad.github.io
Don’t meticulously evaluate and potentially prune every single branch, only to find you have to prune the whole limb anyways.
Or even weirder: conditionals are about figuring out what work doesn’t need to be done. Loops are the “work.”
Ultimately I want my functions to be about one thing: walking the program tree or doing work.
In the lingo, expressions are evaluated by repeatedly getting "rewritten", according to rules called reduction rules. e.g., (1 + 2) + 4 might get rewritten to 3 + 4, which would then get rewritten to 7.
There are two sorts of these rules. There are "congruence" rules, which direct where work is to be done ("which subexpression to evaluate next?"); and then there are "computation" rules (as Pierce [1] calls them), which actually rewrite the expression, and thus change the program state.
"Strict"/"non-lazy" languages (virtually every popular general-purpose language? except Haskell) are full of congruence rules – all subexpressions must be fully evaluated before a parent expression can be evaluated. The important exceptions are special constructs like conditionals and indefinite loops.
For conditionals in particular, a computation rule will kick in before congruence rules direct all subexpressions to be evaluated. This prunes the expression tree, now in a very literal sense.
[1]: Benjamin C. Pierce, Types and Programming Languages (recommended!)
I used to try and form these kinds of rules and heuristics for code constructs, but eventually accepted they're at the wrong level of abstraction to be worth keeping around once you write enough code.
It's telling they tend to resort to made up function names or single letters because at that point you're setting up a bit of a punching bag with an "island of code" where nothing exists outside of it, and almost any rule can make sense.
-
Perfect example is the "redundancies and dead conditions" mentioned: we're making the really convenient assumption that `g` is the only caller of `h` and will forever be the only caller of `h` in order to claim we exposed a dead branch using this rule...
That works on the island, but in an actual codebase there's typically a reason why `g` and `h` weren't collapsed into each other to start.
Aren't you just saying "Real code is more complicated than your toy example"?
Well sure, trivially so. But that's by design.
> Perfect example is the "redundancies and dead conditions" mentioned: we're making the really convenient assumption that `g` is the only caller of `h` and will forever be the only caller of `h` in order to claim we exposed a dead branch using this rule...
Not really. He's just saying that when you push conditional logic "up" into one place, it's often more readable and sometimes you might notice things you otherwise wouldn't. And then he created the simplest possible example (but that's a good thing!) to demonstrate how that might work. It's not a claim that it always will work that way or that real code won't be more complicated.
I spelled out the problem pretty clearly.
> I used to try and form these kinds of rules and heuristics for code constructs, but eventually accepted they're at the wrong level of abstraction to be worth keeping around once you write enough code.
It's the wrong level of abstraction to form (useful) principles at, and the example chosen is just a symptom of that.
I'm not sure why we're acting like I said the core problem with this article is that it uses simple examples.
Your argument sounds like, "I'm so smart and enlightened, I've moved beyond simple heuristics like this." Okay, but the author is also a smart, experienced programmer and is apparently still finding them useful. I am also experienced, and personally find them useful.
I'm not against some argument that there is actually an even better, deeper way to look at these things. But you didn't make that argument. And, perhaps unfairly (you tell me) I suspect your response to that will be that it's all too gossamer, or would take too long to explain....
It doesn't get tedious going through life like that, speaking for two?
Functions to me are more about scoping things down than about performing logic. The whole program is about performing logic.
Like how the phrase “to boldly go where no man has gone before” will bring out pendants.
If I had to hazard some kind of heuristic with 99% applicability, it'd be to always strive to have code with as few indentations (branches) as possible. If your code is getting too indented, those deep Vs are either a sign that your implementation has a strong mismatch with the underlying problem or you need to break things up into smaller functions.
I don’t even like loops and prefer to functionalize them and run in parallel if sensible.
I know this makes me a bit of a python heathen but my code runs fast as a result.
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.
If you find a bug, you find it because you discover that a given input does not lead to the expected output.
You have to find all those ifs in your code because one of them is wrong (probably in combination with a couple of others).
If you push all your conditionals up as close to the input as possible, your hunt will be shorter, and fixing will be easier.
https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-va...
If instead of validating that someone has sent you a phone number in one spot and then passing along a string, you can as easily have a function construct an UncheckedPhoneNumber. You can choose to only construct VerifiedPhoneNumbers if the user has gone through a code check. Both would allow you to pluck a PhoneNumber out of them for where you need to have generic calling code.
You can use this sort of pattern to encode anything into the type system. Takes a little more upfront typing than all of those being strings, but your program will be sure of what it actually has at every point. It's pretty nice.
You just can't enforce those assumptions.
You don’t need to know all of the call chains because you’ve established a “narrow waist” where ideally all things have been made clear, and errors have been handled or scoped. So you only need to know the call chain from entry point to narrow waist, and separately narrow waist till end.
This idle conjecture is too rife with counterexamples.
- If the function is called from 37 places, should they all repeat the if statement?
- What if the function is getaddrinfo, or EnterCriticalSection; do we push an if out to the users of the API?
I think that we can only think about this transformation for internal functions which are called from at most two places, and only if the decision is out of their scope of concern.
Another idea is to make the function perform only the if statement, which calls two other helper functions.
If the caller needs to write a loop where the decision is to be hoisted out of the loop, the caller can use the lower-level "decoded-condition helpers". Callers which would only have a single if, not in or around a loop, can use the convenience function which hides the if. But we have to keep in mind that we are doing this for optimization. Optimization often conflicts with good program organization! Maybe it is not good design for the caller to know about the condition; we only opened it up so that we could hoist the condition outside of the caller's loop.
These dilemmas show up in OOP, where the "if" decision that is in the callee is the method dispatch: selecting which method is called.
Techniques to get method dispatch out of loops can also go against the grain of the design. There are some patterns for it.
E.g. wouldn't want to fill a canvas object with a raster image by looping over the image and calling canvas.putpixel(x, y, color). We'd have some method for blitting an image into a canvas (or a rectangular region thereof).
the idea here is probably that in this case we might be able to split our function into two implementing true and false branches and then call them from 21 and 16 places respectively
You can achieve it by turning the if part into an inline function.
Before:
function(cond, arg)
{
if (cond) { true logic } else { false logic }
}
inline function(cond, arg) { cond ? function_true(arg) : function_false(arg) }
On examples where you're talking about a library function, I think you have to accept that as a library you're in a special place: you're on an ownership boundary. Data is moving across domains. You're moving across bounded contexts, in DDD-speak. So, no, you look after your own stuff.
EnterCriticalSection suggests a code path where strong validation on entry - including if conditions - makes sense, and it should be thought of as a domain boundary.
But when you're writing an application and your regular application functions have if statements, you can safely push them out. And within a library or a critical code section you can move the `if` up into the edges of it safely, and not down in the dregs. Manage your domain, don't make demands of other people's and within that domain move your control flow to the edge. Seems a reasonable piece of advice.
However, as ever, idioms are only that, and need to be evaluated in the real world by people who know what they're doing and who can make sensible decisions about that context.
I'm guessing you mean something else, or do you feel useful functions can't be called many times in the same program?
Unless there's an actual performance implication, this is all purely a matter of taste. This is the kind of broadly true, narrowly false stuff that causes people to destroy codebases. "I can't write it this way, because I have to push ifs up and fors down!!!" It's a totally fake requirement, and it imposes a fake constraint on the design.
static inline int function(blob *ptr, int arg)
{
if (ptr == NULL)
return ERR_NULL;
return real_function(ptr, arg);
}
free(NULL); // convenient no-op, does nothing
fflush(NULL); // flush all streams; done implicitly on normal exit
time(NULL); // don't store time_t into a location, just return it
strtol(text, NULL, 10); // not interested in pointer to first garbage char
setbuf(stream, NULL); // allocate a buffer for stream
realloc(NULL, size); // behave like malloc(size)
sigprocmask(SIG_UNBLOCK, these_sigs, NULL); // not interested in previous signal mask
CreateEventA(NULL, FALSE, FALSE, NULL); // no security attributes, no name
Really?
I do not have to think hard before I have a counter exampl: authentication
I call authenticate() is some form from every API
All 37 of them
I'm not sure if my response is serious or tongue-in-cheek. Maybe a bit of both.
No you aren't. You aren't really calling it from anywhere. The framework you're using, which you aren't writing, is calling the registered middleware.
The topic here is complexity for the code structure because it's called from 37 different places. A registered middleware doesn't run into that issue because it doesn't get called anywhere that "code structure complexity" matters.
Your reasoning is isomorphic to "I'm calling a bit shift millions of times because I have written some code in a programming language." Technically true but not what we're talking about here.
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.
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...
if (weShouldDoThis()) {
doThis();
}
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.
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.
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
//list = [];
killHumans(list);
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?
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.
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"™.
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...
https://en.wikipedia.org/wiki/Unicode_subscripts_and_supersc...
Like for example, if you want to make an idempotent operation, you might first check if the thing has been done already and if not, then do it.
If you push that conditional out to the caller, now every caller of your function has to individually make sure they call it in the right way to get a guarantee of idempotency and you can't abstract that guarantee for them. How do you deal with that kind of thing when applying this philosophy?
Another example might be if you want to execute a sequence of checks before doing an operation within a database transaction. How do you apply this philosophy while keeping the checks within the transaction boundary?
Depends just how many things are checked by the check I guess. A single aspect, checking whether the resource is already claimed or is available, could be combined since it could be part of the very access mechanism itself where anything else is a race condition.
* I wasn’t around long enough to see if there was a hidden maintenance cost
* It was a very thoughtfully designed library in an already-well-understood domain so it wasn’t like we were going to need to change the arguments a ton
* It was explicitly a library designed to be used as a library from the get-go, so there was a clear distinction of which functions should be user-visible.
I think I would find it annoying if I was doing exploratory programming and expected to change the arguments often. But, in that case, maybe it is too early to start checking user inputs anyway.
> If you push that conditional out to the caller, now every caller of your function has to individually make sure they call it in the right way to get a guarantee of idempotency
In this situation your function is no longer idempotent, so you obviously can’t provide the guarantee. But quite frankly, if you’re having to resort to making individual functions implement state management to provide idempotency, then I suspect you’re doing something very odd, and have way too much logic happening inside a single function.
Idempotent code tends to fall into two distinct camps:
1. Code that’s inherently idempotent because the data model and operations being performed are inherently idempotent. I.e. your either performing stateless operations, or you’re performing “PUT” style operations where in the input data contains all the state the needs to be written.
2. Code that’s performing more complex business operations where you’re creating an idempotent abstraction by either performing rollbacks, or providing some kind of atomic apply abstraction that ensures partial failures don’t result in corrupted state.
For point 1, you shouldn’t be checking for order of operations, because it doesn’t matter. Everything is inherently idempotent, just perform the operations again.
For point 2, there is no simple abstraction you can apply. You need to have something record the desired operation, then ensure it either completes or fails. And once that happens, ensures that completion or failure is persistent permanently. But that kind of logic is not the kind of thing you put into a function and compose with other operations.
This is why databases have transactions.
> simple example where you're checking if a file exists
Personally I avoid interacting directly with the filesystem like the plague due to issues exactly like this. Working with a filesystem correctly is way harder than people think it is, and handling all the edge-cases is unbelievably difficult. If I'm building a production system where correctness is important, then I use abstractions like databases to make sure I don't have to deal with filesystem nuances myself.
Your transaction needs to encompass all of those operations, not just parts of it.
- You have to push ifs down, because of DRY.
- If performance allows, you should consider pushing fors up, because then you have the power of using filter/map/reduce and function compositions to choose what actions you want to apply to which objects, essentially vectorizing the code.
Pushing ifs down usually prevents vectorization and the cases article mentions are those non-dry where a similar branch has to be multiplied on a ton of functions down in the stack, often because the type is internally tagged.
Feels a lot like "i before e except after c" where there's so many exceptions to the rule that it may as well not exist.
printInvoice(invoice, options) // is much better than
if(printerReady){
if(printerHasInk){
if(printerHasPaper){
if(invoiceFormatIsPortrait){
:
printInvoices(invoices) // much better than
for(invoice of invoices){
printInvoice(invoice)
}
The function printInvoice should print an invoice. What happens if an invoice cannot be printed due to one of the named conditionals being false? You might throw an exception, or return a sentinel or error type. What do to in that case is not immediately clear.
Especially in languages where exceptions are somewhat frowned upon for general purpose code flow, and monadic errors are not common (say Java or C++), it might be a better option to structure the code similar to the second style. (Except for the portrait format of course, which should be handled by the invoice printer unless it represents some error.)
> while code readability is extremely important, encapsulation is much more important
Encapsulation seems to primarily be a tool for long-term code readability, the ability to refactor and change code locally, and to reason about global behavior by only concerning oneself with local objects. To compare the two metrics and consider one more important appears to me as a form of category error.
> demonstrates arrow anti-pattern
Ewwww gross. No. Do this instead:
if(!printerReady){ return; } if(!printerHasInk){ return; } if(!printerHasPaper){ return; } if(!invoiceFormatIsPortrait){ return; }
Way more readable than an expanding arrow.
> printInvoices(invoices) // much better than
But yes, put the loop into its own function with all of the other assumptions already taken care of? This is good.
in Elixirland, we'd name that function maybe_print_invoice which I like much better.
Add asserts to the end of the function too.
Loop's can live in the middle, take as much I/O and compute out of the loop as you can :)
With AVX-512 for example, trivial branching can be replaced with branchless code using the vector mask registers k0-k7, so an if inside a for is better than the for inside the if, which may have to iterate over a sequence of values twice.
To give a basic example, consider a loop like:
for (int i = 0; i < length ; i++) {
if (values[i] % 2 == 1)
values[i] += 1;
else
values[i] -= 2;
}
__mmask16 consequents;
__mmask16 alternatives;
__mm512i results;
__mm512i ones = _mm512_set1_epi32(1);
__mm512i twos = _mm512_set1_epi32(2);
for (int i = 0; i < length ; i += 16) {
results = _mm512_load_epi32(&values[i]);
consequents = _mm512_cmpeq_epi32_mask(_mm512_mod_epi32(results, twos), ones);
results = _mm512_mask_add_epi32(results, consequents, results, ones);
alternatives = _knot_mask16(consequents);
results = _mm512_mask_sub_epi32(results, alternatives, results, twos);
_mm512_store_epi32(&values[i], results);
}
For your example loop, the `if` statements are contingent on the data; they can't be pushed up as-is. If your algorithm were something like:
if (length % 2 == 1) {
values[i] += 1;
} else {
values[i] += 2;
}
In your optimized SIMD version, you've removed the `if` altogether and are doing branchless computations. This seems very much like the platonic ideal of the article, and I'd expect they'd be a big fan!
For a contrived example, we could attempt to be clever and remove the branching from the loop in the first example by subtracting two from every value, then add three only for the odds.
for (int i = 0; i < length ; i++) {
values[i] -= 2;
values[i] += (values[i] % 2) * 3;
}
In regards to moving branches out of the loop that don't need to be there (like your check on the length) - the compiler will be able to do this almost all of the time for you - this kind of thing is standard optimization techniques that most compilers implement. If you are interpreting, the following OPs advice is certainly worth doing, but you should probably not worry if you're using a mature compiler, and instead aim to maximize clarity of code for people reading it, rather than trying to be clever like this.
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.
fn frobnicate(walrus: Option<Walrus>)`) // GOOD
frobnicate_batch(walruses)
// BAD
for walrus in walruses {
frobnicate(walrus)
}
fn frobnicate_batch(walruses)
for walrus in walruses { frobnicate(walrus) }
}
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() }
}
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() }An interface where the implementation can later be changed to do something more clever.
At work we have a lot of legacy code written the BAD way, ie the caller loops, which means we have to change dozens of call sites if we want to improve performance, rather than just one implementation.
This makes it significantly more difficult than it could have been.
Firstly, in many cases the function needs to serve both purposes — called on a single item or called on a sequence of such. A function that always loops would have to be called on some unitary sequence or iterator which is both unergonomic and might have performance implications.
Second, the caller might have more information than the callee on how to optimize the loop. Consider a function that might be computationally expensive for some inputs while negligible for others — the caller, knowing this information, could choose to parallelize the former inputs while vectorizing etc. the latter (via use of inlining, etc.). This would be very hard or at least complicate things when the callee's responsibility.
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)
posts = fetchPosts(postIDs)
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.
Our cloud provider had an aircon/overheating incident in the region we were using, and after it was resolved network latency between the database and application increased by a few milliseconds. Turns out if you multiply that by a few million/fast arrival rate you get a significant amount of time, and the pending tasks queue backs up causing the high priority tasks to be delayed.
Based on the traces we had it looked like a classic case of "ORM made it easy to do it this way, and it works fine until it doesn't" but was unfortunately out of our control being a third party product.
If they'd fetched/processed batches of tasks from the database instead I'm confident it wouldn't have been an issue.
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?
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?
I use `if`s a markers for special/edge cases and typically return in the last statement in the `if` block.
If I have an `else` block and it's large, then it's a clear indicator that it's actually two methods dressed as one.
This is not to say we shouldn't be having conversations about good practices, but it's really important to also understand and talk about the context that makes them good. Those who have read The Innovator's Solution would be familiar with a parallel concept. The author introduces the topic by suggesting that humanity achieved powered flight not by blindly replicating the wing of the bird (and we know how many such attempts failed because it tried to apply a good idea to the wrong context) but by understanding the underlying principle and how it manifests within a given context.
The recommendations in the article smell a bit of premature optimisation if applied universally, though I can think of context in which they can be excellent advice. In other contexts it can add a lot of redundancy and be error prone when refactoring, all for little gain.
Fundamentally, clear programming is often about abstracting code into "human brain sized" pieces. What I mean by that is that it's worth understanding how the brain is optimised, how it sees the world. For example, human short term memory can hold about 7±2 objects at once so write code that takes advantage of that, maintaining a balance without going to extremes. Holy wars, for example, about whether OO or functional style is always better often miss the point that everything can have its placed depending on the constraints.
If a certain function has many preconditions it needs to check, before running, but needs to potentially run from various places in the code, then moving the precondition checks outside the method results in faster code but destroys readability and breaks DRY principle.
In cases where this kind of tension (DRY v.s. non-DRY) exists I've sometimes named methods like 'maybeDoThing' (emphasis on 'maybe' prefix) indicating I'm calling the method, but that all the precondition checks are inside the function itself rather than duplicate logic all across the code, everywhere the method 'maybe' needs to run.
Within a function, I'm a fan of early bail out.
While this goes against the usual advice of having the positive branch first, if the positive branch is sufficiently large you avoid having most of the function indented.
This is advice I've never seen or received. It's always been the latter, exit early, etc. Languages like Swift even encode this into a feature, a la if guards.
Negative first makes else-branches double negative which reads weird, eg. if !userExists {…} else {…}
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.
Pushing fors down is usually not that relevant in Rust if all it achieves is inlining. The compiler can do that for you, or you can force it to, while improving reusability. It can make sense if there's more optimization potential with a loop e.g. lifting some logic outside the loop, and the compiler doesn't catch that. I also would avoid doing this in a way that doesn't work well with iterators, e.g. taking and returning a Vec.
When I work with batches of data, I often end up with functions like this:
function process_batch(batch) {
stuff = setUpNeededHelpers(batch);
results = [];
for (item in batch) {
result = process_item(item, stuff);
results.add(result);
}
return results;
}
So the setUpNeededHelpers() section, while not extremely expensive, can have nontrivial cost.
I usually add a clause like
if (batch.length == 0) {
return [];
}
Also, sometimes the initialization requires to access one element from the batch, e.g. to read metadata. Therefore the check also ensures there is at least one element available.
Wouldn't this violate the rule?
The article is offering a heuristic, not a hard rule (rule of thumb = heuristic, not dogma). It can't be applied universally without considering your circumstances.
Following his advice to the letter (and ignoring his hedging where he says "consider if"), you'd move the `if (batch.length == 0)` into the callers of `setUpNeededHelpers`. But now you have to make every caller aware that calling the function could be expensive even if there are no contents in `batch` so they have to include the guard, which means you have this scattered throughout your code:
if (batch.length == 0) { return default }
setup(batch)
The advice really is contingent on the surrounding context (non-exhaustive):
1. Is the function with the condition called in only one place? Consider moving it up.
2. Is the function with the condition called in many places and the condition can't be removed (it's not known to be called safely)? Leave the condition in the function.
3. Is the function with the condition called only in places where the guard is redundant? In your example, `batch.length == 0` can be checked in `process_batch`. If all calls to `setup` are in similar functions, you can remove the condition from `setup` and move it up.
4. If it's causing performance concerns (measured), and in many but not all cases the check is unneeded then remove the guard from `setup` and add it back to only those call-sites where it cannot be safely removed. If this doesn't get you any performance improvements, you probably want to move it back down for legibility.
Basically, apply your judgment. But if you can, it's probably (but not always) a good idea to move the ifs up.
[0] https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-va...
You'll end up duplicating the condition, but that seems like a reasonable price to pay for correct and performant software.
Nested g() and h() can be much better if they are even just 1% easier to understand. No one cares about a few extra CPU cycles, because we don't write system or database code.
Is this more of a ‘server programming is not systems programming because we are just implementing the logic of what gets served’ vs. my assumption that server programming includes the how does the server connect to the world, cache things, handle resource allocation, and distribute/parallelize the computations required to serve data back to the user?
Maybe I'm using the terminology wrong, and it's actually Applications Programming, but it's easy to confuse with mobile/desktop applications, where RAM does matter. In servers we pay for RAM/CPUs ourselves.
And this was obfuscated by author's use of global variables everywhere.
The key change was reducing functions' dependencies on outer parameters. Which is great.
With conditionals, it's also useful to express them as ternary assignment when possible. This makes it more likely the optimizer will generate a conditional move instead of a branch. When the condition is not sufficiently predictable, a conditional move is far faster due to branch misprediction. Sometimes it's not always faster in the moment, but it can still alleviate pressure on the branch prediction cache.
Fors = data flow / compute kernel
it makes sense to keep control flow and data flow separated for greater efficiency, so that you independently evolve either of flows while still maintaining consistent logic
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)
}
}
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.
// Good? for walrus in walruses { walrus.frobnicate() }
Is essentially equivalent to
// BAD for walrus in walruses { frobnicate(walrus) }
And this is good,
// GOOD frobnicate_batch(walruses)
So should the first one really be something more like
// impl FrobicateAll for &[Walrus] walruses.frobicate_all()
But I like to help the compiler with this kind of optimization, by just doing it in the code. Let the compiler focus on optimizations that I can't.
anywhere else, push ifs up.
99% of the time you can write better code without it.
This part in particular seems like an aesthetic judgment, and I disagree. I find it more natural to follow a flowchart than to stare at one.
> A related pattern here is what I call “dissolving enum” refactor.... There are two branching instructions here and, by pulling them up, it becomes apparent that it is the exact same condition, triplicated (the third time reified as a data structure):
The problem here isn't the code organization, but the premature abstraction. When you write the enum it should be because "reifying the condition as a data structure" is an intentional, purposeful act. Something that empowers you to, for example, evaluate the condition now and defer the response to the next event tick in a GUI.
> The primary benefit here is performance. Plenty of performance, in extreme cases.
Only if so many other things go right. Last I checked, simply wanting walruses to behave polymorphically already ruins your day, even if you've chosen a sufficiently low-level programming language.
A lot of the time, the "bad" code is the implementation of the function called in the "good" code. That makes said function easier to understand, by properly separating responsibilities (defining frobnication and iterating over walruses). Abstracting the inner loop to a function also makes it sane to express the iteration as a list comprehension without people complaining about how you have these nested list comprehensions spread over multiple lines, and why can't you just code imperatively like the normal programmers, etc.
> The two pieces of advice about fors and ifs even compose!
1. The abstraction needed to make the example comprehensible already ruins the illusion of `frobnicate_batch`.
2. If you're working in an environment where this can get you a meaningful performance benefit and `condition` is indeed a loop invariant (such that the transformation is correct), you are surely working in an environment where the compiler can just hoist that loop invariant.
3. The "good" version becomes longer and noisier because we must repeat the loop syntax.
> jQuery was quite successful back in the day, and it operates on collections of elements.
That's because of how it allowed you to create those collections (and provided iterators for them). It abstracted away the complex logic of iterating over the entire DOM tree to select nodes, so that you could focus on iterating linearly over the selected nodes. And that design implicitly, conceptually separated those steps. Even if it didn't actually build a separate container of the selected nodes, you could reason about what you were doing as if it did.
The premise that you can define best patterns like this, removed from context with toy words like frobnicate, is flawed. You should abstract your code in such a way that the operations contained are clearly intuited by the names and parameters of the abstraction boundaries. Managing cognitive load >>> nickle and dime-ing performance in most cases.