4

I'm looking to improve the readability of a lengthy C++ function. This function contains a number (> a dozen) variables that are used throughout. The main logic of the code is a long list of condition checks (sometimes nested, sometimes with loops) followed by various actions. Because of the lengthy number of lines detailing the actions to be performed mixed in with the logical/looping glue, it can be quite hard to follow what's going on. There's no simple way to siphon off these actions into separate functions without arbitrary, ugly function signatures noting whatever subset of variables happen to be needed. One drastic solution might be to replace the function with a singleton class, with the function's variables and sub-actions becoming private member variables and private member. However, I'm looking for a simpler alternative. My idea is to define a list of actions at the top of my function through lambda functions, and then below this perform the function's logic using these one-off lambdas. This would look something like (entirely schematically);

void function() {
    // variables
    int a, b, c, d, ...;

    // actions to be performed
    auto func1 = [&] () { ... code using a,b, etc. };
    auto func2 = [&] () { ... };
    auto func3 = [&] () { ... };
    ...

    // main logic
    if (<condition 1>) {
        if (<condition 2>)
            func1();
        else
            func2();
    } else {
        func2();
        func3();
    }
    ... // etc
}

These lambda functions would occasionally save code, in the the cases where they replace repeated code fragments, but would usually just improve readability -- at least to my eyes. Is this a good practice in general? Do others find that this improves readability, and what is the cost of using these lambda functions?

  • 1
    So is the idea here that func1() has access to a without needing it passed in? – candied_orange Jul 2 '17 at 14:58
  • @CandiedOrange Yes, that's the idea -- I should make that clearer – jwimberley Jul 2 '17 at 14:59
  • 5
    Seems like you're doing what object oriented does with private methods. Technically these are closures except you didn't return them you just started using them. It's certainly unusual. I don't like to deal with unusual unless I'm getting something out of it. What am I getting out of this? – candied_orange Jul 2 '17 at 15:46
7

Does it improve readability ?

Your way of using lambdas to break-down a larger function in smaller parts is similar to the nested functions in Pascal, ADA and other languages.

It indeed improves the readability of the main part of your function body: there are less statements to read to understand what it does. This is the main purpose of nested functions. Of course, I assume that nowadays, most programmers are familiar with the syntax of lambdas.

However, is it a good idea ?

Scott Meyers, in his book Effective Modern C++ warns against the use of default capture in lambdas. His main worry is about dangling references (e.g. if a lambda is defined in a block and is used out of the scope of this block when the variable doesn't exist anymore), which seems not to be an issue in your case.

But he also underlines another problem: the illusion of having a self-contained function. And here lies the major weakness of your approach:

  • you have the impression that your lambda is self contained, but in fact it's completely dependent of the rest of the code, and you don't see easily in your lambda where the captured values are coming from, which assumptions you can make on them, etc...
  • as the link with the main body is based on the captured variables, which can be read or written, it is in fact very difficult to guess all the side effects hidden in your lambda invocation, which could influence your main part.
  • so it's very difficult to identify assumptions and invariants in the code, both of the lambda, and of your mega function
  • in addition, you could accidentally change a variable that you forgot to declare locally in your lambda, and one happens to have the same name in the function.

First advice: at least, enumerate explicitly the variables captured by your lambda, in order to better control the potential side effects.

Second advice: once this works, you could think of strengthening your structure further, by evolving from capture to parameter passing. If there are too many of them, you'd have to refactor. One approach could be to make your function a callable class, promoting your throw away lambdas to member functions, and making the variables used throughout the computation member variables. But it's difficult to say if it's the best option from the elements you gave.

And why are you in such a situation ?

The next think to think about, is why you have such a big function in the first place. If you'd follow Uncle Bob's advice given in his book Clean Code (summary of the function topic on this blog page) you should have:

  • small functions,
  • that do one thing (single responsibility),
  • and that do only things at one level of abstraction
  • Note that the "Single Responsibility Principle" refers to classes or modules, not methods or functions, and that "Single Responsibility" in this context does not mean "do one thing." – Robert Harvey Jul 3 '17 at 0:22
  • 1
    I like this advice, and I appreciate the thought you put into it. I think I'll put your first and second advice into practice right away: making lambdas with explicit captures, and (as a first step) identifying actions that are variations on a theme and which could be implemented through a stand-alone function with different variable sets passed to it as needed. A callable (singleton) class is my eventual goal, though this will take some refactoring. This is legacy code, but thanks also for the Clean Code link and recommendation -- I don't want my own code to become problematic in the future! – jwimberley Jul 3 '17 at 1:43
  • 1
    @RobertHarvey Single responsability principle also applies to functions. The thing that changes is the scope... – Phil1970 Jul 3 '17 at 2:12
2

This will make your code even more unreadable, as it will be unclear what is read/written to in each action. Trying to hide the passing of variables makes code fragile and rigid. It should be avoided. As far as performance goes, a good compiler should be able to unrawp the lambda functions into inline code without a performance penalty. You could check the generated assembly to verify this.

The best way to solve these issues, is to sit down with someone, and explain the code to them. In this process you will identify high-level concepts and operations happening. Break these out to be separate functions (this should also help you determine meaningful function names).

To solve the issue of many input and many output variables of a function, you can use structs to pass in/out tightly coupled variables (this will improve readability):

struct foo_params {
  int numX;
  float ratioY;
  int numZ;
  float const * data;
  /* many more parameters */
};

void compute(
    struct foo_params * const params)
{
  /* operate on data */
}

Note that in some situations this may have a performance impact so you should do some testing if this is performance sensitive code.

  • Thanks for your advice. I see the benefit of passing structs and I'll look to see where I can use this -- though it might be hard to break down the variables modularly into thematically-grouped structs, it will be good for me to understand the behavior of this legacy code backwards and forwards, and to group its parameters/variables as modularly/thematically as possible. – jwimberley Jul 3 '17 at 1:49
  • 1
    At that point, it might be better to move most of the code in the structure itself (and use a class instead where implementation functions are private) – Phil1970 Jul 3 '17 at 2:15

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.