Skip Navigation
22 comments

  • 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:

     rust
        
//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 

     rust
        
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!)

22 comments