Efficiency for Style?

[Aaron Vienneau]

Generally, I like to break down my if statements into predicate functions for clarity and neatness.

A typical function I might write looks like this:

void Nest::breed()
{
  auto isMale = [](Bunny b){ return b.getSex() == Bunny::Sex::MALE; };
  auto isAdult = [](Bunny b){ return b.getAge() >= 2; };
  auto isFertile = [](Bunny b){ return b.getRadioactiveStatus() != true; };

  for(auto maleItr = colony.begin(); maleItr != colony.end(); maleItr++)
    if(isMale(*maleItr) && isAdult(*maleItr) && isFertile(*maleItr))
      for(auto femaleItr = colony.begin(); femaleItr != colony.end(); femaleItr++)
        if(!isMale(*femaleItr) && isAdult(*femaleItr) && isFertile(*femaleItr))
        {
          colony.push_back(Bunny(femaleItr->getColor()));
          checkFoodShortage({maleItr, femaleItr});
        }
}

I'd think that would (1) take up more memory on the stack (because you're saving lambdas into variables and (2) increases execution time, because control is passed several times between the different assorted functions UNLESS those can be inlined for some reason (which I doubt the compiler would do.

Is the clarity worth the efficiency loss? Is this a typical layout for a function you might see? Any suggestion on how to make it better?

Obviously I could break that down into a huge long, nested loop/if statement, but it just looks SUPER ugly. . .

[Peter87]

Since you are not capturing anything in the lambdas it should not be any worse than just calling a regular function. Both Clang and GCC have no problem inlining the calls to these lambdas. My only worry is that you are passing the Bunny by value, leading to unnecessary copying.

[Aaron Vienneau]

Since you are not capturing anything in the lambdas it should not be any worse than just calling a regular function

What are the consequences of including names in the capture list? I know that capturing brings names into scope, but I didn't know there was a consequence in efficiency.

[ne555]

when you do colony.push_back(Bunny(femaleItr->getColor())); you don't care about the male, so you only need a quantity.
that could make the algorithm O(n) instead of O(n^2)
or if it does matter, you may have separate lists for male and female, to avoid checking them all.
that's the effency you should worry about.

about clarity, I was expecting a can_breed(a, b) function.

for(auto a: colony)
   for(auto b:colony)
      mating(a,b) //the result depends on fertility and sex 

[jonnin]

I guess readability is subjective, but to me,

if( object.sexget() == MALE && object.agegett() >= 2 ... etc)
is more readable than creating 3 difficult to read statements that are then referenced in the if condition. The reader has to back-reference all that junk rather than just see directly what exactly is happening right there where the code is. It is also more challenging to tune for performance if you DO decide you need it fast. I see no benefits (including easier to read) from doing it this way HERE. If the conditionals were complicated enough (it would take quite a bit of logic to justify it) then doing it this way will eventually overtake a gigantic messy condition statement, but for what you posted, it looks over-engineered and due for a KISS rewrite.

The only saving grace is that you used great names, so its easy to understand what the code is doing at a high level, but that doesn't change that debugging / modifying/ tuning it is still going to be more work to understand it at the low level. And presumably your getters have equally good names, so that isnt an improvement over the straight if-statement.

/shrug I could be in the minority here. But it seems to me you can eliminate all 3 expressions and write a fairly simple (if slightly longer) conditional and have a cleaner overall product.

[icy1]

partially agreed with ne555 -- separate lists for male and female would make this part simpler to read, though not necessarily lesser complexity, since it looks like every male is hitting up every female for a booty call -- N*M unavoidable.

On naming, though, i'd remove the "get" words, change the radioactive method to sound more boolean, like IsRadioactive(), and combine the full idea of what you want into one. Prob also want to stop breeding when food shortage.

void Nest::Breed()
{
  auto IsSexyMale = [](const Bunny& b)
  { 
    return b.Sex() == Bunny::Sex::MALE && 
           b.Age() >= 2 &&
           !b.IsRadioactive();
  };

  auto IsSexyFemale = [](const Bunny& b)
  { 
    return b.Sex() == Bunny::Sex::FEMALE && 
           b.Age() >= 2 &&
           !b.IsRadioactive();
  };

  for (const auto& a : colony)
  {
    if (IsSexyMale(a))
    {
      for (const auto& b : colony)
      {
        if (IsSexyFemale(b))
        {
            colony.emplace_back(Bunny(b.Color()));
            if (FoodShortage())
                return;    
        }
      }
    }
  }

}

[Peter87]

What are the consequences of including names in the capture list? I know that capturing brings names into scope, but I didn't know there was a consequence in efficiency.

You are essentially creating an object that stores the captured variables as members, and that has an operator() that contains the code that you wrote in the lambda expression. This is technically true for non-capturing lambdas as well but since they can be implicitly converted to a function pointer I think it's quite obvious that they will perform the same as a function pointer and that calling it would just be a regular function call.

I'm not saying a capturing lambda is necessarily less efficient. Compilers are very good at optimizing and inline local lambdas.

[Aaron Vienneau]

I guess readability is subjective, but to me

That's good feedback though. It does feel a tad over-engineered which is why I asked initially. I feel as though if the expressions get a bit more complicated it might be easier to read them if they're broken up into named lambdas, like this. . . but making them member functions or stand-alone functions seems a bit overboard, cause they're only really used that once.

Also, I like my code to be fairly self documenting, which is why I try to stay away from "magic numbers" and instead try to encapsulate ideas in named lambdas. For instance, if I saw,

if(bunny.getAge() > 2) {} I'd think, "Ok, so we're checking to see if the bunny's age is over 2, but why?" In the context of this program it means that the bunny is an adult, in the context of another program it could mean something entirely different. Granted, that could possibly be simplified with a named constant like if(bunny.getAge() > AGE_OF_CONSENT){} so it really is a preferential thing and I value your feedback.

I'm not saying a capturing lambda is necessarily less efficient. Compilers are very good at optimizing and inline local lambdas. Good to know :)

They say you write good code by first imitating those who write good code, so I'm just trying to get a sense of how to balance efficiency with readability and elegance and maintainability and all that stuff...

Thanks!