I disagree with this article.

The first one is a good point in principle, but I would call it pushing down the ifs. A method is usually used to group logic together. So pulling the assertion checks out of that method forces you to replicate it every single time before calling the function. It’s very likely you forget to update these conditions in one place and not worth the miniscule loss of performance.

Now, if frobnicate is only something a walrus can do, then it should take a walrus. But if it is something like “given a picture of an animal, check if it is of a walrus, determine its sex and if it is a male, then give the likelihood that is the dominant male on this part of the beach” then you should most certainly push the ifs down to contain all that logic in one single frobnicate method.

Regarding the loops:

If the example given is really all the code you have, then sure, pull that condition out of the loop if you want. But the compiler will optimize it anyway if the condition does not change every loop iteration. And if it does you can’t pull the condition out of the loop anyway. A for loop I have much more often in my code looks like this though:

//BAD?
for walrus in walruses { 
    if !walrus.has_tracker() {
        continue;
    }

    if weather.is_bad() {
        continue;
    }


    let weight = estimate_weight(walrus);
    if weight <= threshold_weight {
        continue;
    }

    if study.is_about_frovnication { 
        walrus.frobnicate() 
    } else { 
        walrus.transmogrify()
    } 
} 

Which you do not want to spread to two for loops! Sure, in this toy example the if conditions could be simplified. And if you have such trivial loops, you should use iterators anyway. But sometimes you have logic you need to apply to every element, do not want to put that logic in the frobnicate method (especially if you push your ifs up) and can’t use iterators in an elegant way.

For the love of god, do not turn it into

if study.is_about_frobnication {
for walrus in walruses { 
    if !walrus.has_tracker() {
        continue;
    }

    if weather.is_bad() {
        continue;
    }


    let weight = estimate_weight(walrus);
    if weight <= threshold_weight {
        continue;
    }

walrus.frobnicate()
} else {
for walrus in walruses { 
    if !walrus.has_tracker() {
        continue;
    }

    if weather.is_bad() {
        continue;
    }


    let weight = estimate_weight(walrus);
    if weight <= threshold_weight {
        continue;
    }

    walrus.transmogrify()
}

And last but not least, Profile before you optimize anything. The part about the ifs, and the enum example may not actually impact your performance at all! The compiler is very smart about moving code around, so the enum thing would most likely be inlined and then reduced down further, potentially eliminating the enum from the code completely. It can still be a worthwhile code change to make, because it improves legibility. But since the author mentioned the performance a few times I wanted to give a counterpoint. (That being, profile before you optimize!)

Saw this posted on hackernews yesterday, along with hundreds of comments of people completely misunderstanding the advice given. Glad to not see any of that here.

I agree with part of the article, because I didn’t read the rest. I truly dislike the use of single letter variable names: f, g, h and foo, bar, baz. My advice: use descriptive variable names.

function twoIfs, function complicatedIf, var simpleAnd, etc. Makes it so much easier to read examples instead of remembering “oh yeah, f had two ifs in it, h had the if/else, g calls f which calls h which,…”.

Also see this often in other examples: "A for ‘Truthy variable’ " 😓 Wtf. Laziness is good when it makes things easier, not harder.

What’s up with that capitalisation, I thought it was about git lfs at first, really confused me lol

I’m curious, is there a difference in doing it like this?:

var condition = var1 && var2

for walrus in walruses {
  if (condition) {
    walrus.frobnicate()
  } else {
    walrus.transmogrify()
  }
}

Doesn’t assigning the condition to a variable make it so that it’s only really evaluated once?

Seems more applicable to an imperative style, and IMO even still the advice is too dependent on special/actual case details to be generally applicable as a “rule of thumb”.

This is just one specific example amongst many of how redundant logic could be simplified because sometimes the branch is an implementation detail and you want to push it down, and sometimes it’s not and you want to push it up.