6

There are a bunch of methods in a class that I want to clean up. These just build up a data structure (with different values) over and over again and add them to a container passed in, like so:

SomeClass::Foo(ContainerType& cont)
{
   AnotherType dataItem;
   dataItem.item1 = true;
   dataItem.item2 = 42;
   // blah blah
   cont.addItem(dataItem);

   // ... snip ...

   AnotherType newItem;
   newItem.item1 = false;
   newItem.item2 = 128;
   // blah blah
   cont.addItem(newItem);
}

It is rather convenient (relevant code within same function, so easy to get to and read) to have a lambda function local to each method and call it instead (future changes will be limited to one place), like so:

SomeClass::Foo(ContainerType& cont)
{
   auto buildData = [](ContainerType& cont, bool flag, int value){
       AnotherType dataItem;
       dataItem.item1 = true;
       dataItem.item2 = 42;
       // blah blah
       cont.addItem(dataItem);
   };
   buildData(cont, true, 42);
   // ... snip ...
   buildData(cont, false, 128);
}

rather than adding a new private member function for doing the same:

SomeClass::Foo(ContainerType& cont)
{
    buildData(cont, true, 42);
    buildData(cont, false, 128);
}
SomeClass::buildData(ContainerType& cont, bool flag, int val)
{
   // blah blah
}

Given that the internals of how to build AnotherType objects is not useful/necessary outside the method SomeClass::Foo(), here are the questions I seek guidance for:

  • Are there any other cleaner/better solutions to this?
  • Are there any non-obvious pitfalls to using the local lambda approach over the new private member approach?

Currently, I've used the local lambda approach and cut a lot of the repeating code out. I just want to ensure that this is an acceptable way to go.

  • Define acceptable. It's very unusual as an idiom (so far), and I would consider it quite surprising if someone in my team did that. Is that a reason for it to not be acceptable? – Sebastian Redl Aug 13 '15 at 10:10
  • Ok, just wanted to ensure that it isn't obviously wrong in places I didn't think about. The code in question has a bunch of conditions and calls to other classes, which make it less obvious that all the repeated cruft is really not necessary. – Bhargav Bhat Aug 13 '15 at 10:20
  • 2
  • 2
    FYI, if you put &cont inside the square brackets [], it closes over that variable, making it available inside the lambda without having to pass it to every call. – Karl Bielefeldt Aug 13 '15 at 13:12
  • My first thought on seeing this change is "That's exactly what I would've done in Javascript." – Ixrec Aug 30 '15 at 14:07
5

Both solutions make sense, and have different tradeoffs.

Lambdas

The style of using lambdas inside another function is very common across many languages. If it seems alien to you, that's just because lambdas weren't available in C++ until recently. The advantage of this style is that the whole implementation of the function is within that function, with the lambda grouping repeated behaviour into a single block. Since the lambda can capture local variables (cont in your case), you will not typically have to pass many arguments. Drawbacks are that this function can still grow rather longish, and that you can't independently test the extracted behaviour.

Named functions

If you use a proper function (either a free function in an anonymous namespace, or a private member function), then your main function becomes less cluttered, and the extracted behaviour is independently testable. This extract-function refactoring is incredibly common, and makes it a breeze to test and understand the code: Each function is only a couple of lines long. And with proper names, such code becomes highly self-documenting.

However, parts of the behaviour are now strewn all over the place, and forming a complete understanding of the behaviour requires a reader to stitch together all these bits and pieces – too much abstraction can result in a loss of maintainability. Also, the previously internal code can be now called from any other function within its scope, which means that invariants that are obvious within local usage are now not guaranteed and have to be re-checked if you are interested in correct code, e.g. asserting that a pointer is non-null. Finally, an important downside is that any context which the function operators on has to be passed explicitly as an argument. This can become tedious for 5–8 arguments, since C++ does not have named formal parameters :(

For-loops

Another possibility, more like creating a lambda, is storing the data you are operating on in a vector, and then performing an action on each item via a loop. E.g.:

std::vector<WorkItem> items {
  {true, 42},
  {false, 128},
};
for (const auto& item : items) {
  ...
}

Often, the work item type might just be a tuple or pair. I'm not too fond of this solution since the ordering of the code is a bit unintuitive, but it's an effective method to get repeated code out of the way.

Personal opinion

When I extract behaviour into a separate function, I first tend to use a lambda since the advantage of capturing local variables is tremendous. However, in later refactorings I tend to use named functions so that I can test them more easily. For me, this outweighs the cost of losing context.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.