Rule suggestion: try to keep special-case handler close to special-case test

[sts1skj]

This is a rule that I've been following for many years that, in my experience, makes code less error-prone. Here's some suggested text:

(start suggested text)

Try to keep code for handling a special case close to the test for that special case.

Example:

// bad
int parse_subdata3(const JSONObject& obj)
{
    const JSONObject& sd3;
    if (obj.find_subobject("subdata3", sd3))
    {
        int error_count = 0;
        // 300 lines of code for dealing with sd3, counting errors along the way.
        return error_count;
    }
    else
    {
        std::cerr << "Subobject subdata3 not found\n";
        return 1;
    }
}

// good
int parse_subdata4(const JSONObject& obj)
{
    const JSONObject& sd4;
    if (!obj.find_subobject("subdata4", sd4))
    {
        std::cerr << "Subobject subdata4 not found\n";
        return 1;
    }
    int error_count = 0;
    // 300 lines of code for dealing with sd4, counting errors along the way.
    return error_count;
}

Reason: A special case is one thought, not two. The testing for a special case and the code for handling that special case should be together to reduce confusion. When the two are far apart, it's easy to update one and forget to update the other; it's easy to miss the fact that there's an actual handler for that special case much farther along in the program; it's also easy to mistake just which piece of code is the special-case handler. In addition, handling a special case up front, if the special-case handler ends with a "return", "continue", or "throw" statement, may allow you to reduce the indentation level of the non-special-case code, improving readability and ease of comprehension.

Another example:

// bad
for (const auto& rec : records)
{
    if (!rec.special_flag_7)
    {
        // 300 lines of code
    }
    // Note: the else clause would be here, if one were needed for handling the special case.
}

// good
for (const auto& rec : records)
{
    if (rec.special_flag_7)
        continue;
    // 300 lines of code
}

Note that in this example the way of handling the special case is to do nothing with the record, that is, to continue with the loop. In the 'bad' version, the special-case handler is 300 lines away from the test for it.

Exception: If the size or complexity of the special-case-handling code is on the order of the size or complexity of the non-special-case-handling code, it makes little difference which comes first (from the point of view of this rule).

See also: "ES.77: Minimize the use of break and continue in loops". Note that this rule is an important exception to ES.77. You should use continue when it is appropriate for getting a special case out of the way at the beginning of a loop.

(end suggested text)

I've seen hundreds of bugs over the years that are the result of someone not following this rule. Fixing the code to follow the rule usually makes the bug obvious.

Please forgive me if this rule already exists, has already been proposed, or has already been discussed. I spent a lot of time searching the rule list and the issue list and found nothing. Since it's such a simple rule, I'd expect it to be here already.

[sts1skj]

Also see issue #1765

[beinhaerter]

From what I can see the major point in your examples is already handled by F.56: Avoid unnecessary condition nesting!?

[shaneasd]

Also seemingly related: NR.2: Don’t insist to have only a single return-statement in a function (the example more so than the title)

[sts1skj]

You're right: Both F.56 and NR.2 overlap with my suggested rule. (I apologize for somehow missing them in my search.) I strongly support both F.56 and NR.2. But, as currently written, F.56 and NR.2 don't completely cover my suggested rule.

F.56 needs an example to show that it applies to continue and not just return. The wording of the last sentence of the "Reason" clause could be improved: it should imply that minimizing nesting is the goal, not necessarily getting all the way to the outermost scope. I'd suggest changing it like this: "Strive to place the essential code at outermost scope" -> "Strive to minimize the scope nesting of the essential code". Furthermore, F.56 should probably be in the "Expressions and statements" section rather than the "Functions" section, as it applies to more than just a function's outermost scope.

NR.2 needs a companion rule with a title like, "Don't avoid using continue." The explanation would be very similar to NR.2 but would cover the inappropriate avoidance of continue instead of return.

But even if F.56 and NR.2 were improved as I suggest in the previous two paragraphs to increase their overlap with my "special-case" rule suggestion, I think my suggested rule would still be valuable. The reason is that the concept is different. In F.56, the emphasis is on minimizing nesting of the essential code. In NR.2, the emphasis is on arguing against the "single return statement" rule. Neither rule is about thought cohesion through keeping special-case handling close to the special-case test.

Here's my suggestion for a new rule to accompany NR.2 but focusing on 'continue' instead of 'return':

(start suggested rule 2)

Don't avoid using continue

Reason: The avoid-continue rule can lead to unnecessarily convoluted code. In particular, the avoid-continue rule makes it harder to concentrate error checking and other special-case handling at the top of a loop.

Example:

// good
for (const auto& rec : records)
{
    if (rec.special_flag_5)
        continue;
    int priority = calculate_priority(rec);
    if (priority < 7)
        continue;
    // do something with rec and priority
}

If we rewrite this code to avoid continue, we would probably do something like:

// bad
for (const auto& rec : records)
{
    if (!rec.special_flag_5)
    {
        int priority = calculate_priority(rec);
        if (priority >= 7)
        {
            // do something with rec and priority
        }
    }
}

This is longer and more difficult to understand. Consider where you might add code for doing something when rec.special_flag_5 is true: you might add an else clause after the second of the three closing braces. Yuck.

Exception: Using continue deep in the middle of a long loop, nested two or more scopes deep, can be hard to spot, and thus can be error-prone. See "ES.77: Minimize the use of break and continue in loops".

See also: Rule "ES.??: Try to keep code for handling a special case close to the test for that special case" and rule "F.56: Avoid unnecessary condition nesting".

(end suggested rule 2)

Now that I think about it, the "reason" given in rule "ES.77: Minimize the use of break and continue in loops" doesn't support the minimization of continue, just break. This rule could stand to be reworked and possibly split in two, as the rule for continue is naturally going to be different from the rule for break. As currently written, rule ES.77's advice for continue seems to contradict F.56 and both of my suggested rules.

[shaneasd]

I'm not sure I agree with your continue example. I find the "bad" code easier to read. I think you could make an argument for special_flag_5 being a special case early exit but at the point where you have to do work to calculate the priority (I'm assuming this is potentially a stand in for multiple lines of calculation) I think the subsequent continue is too deep into the function. My preference is that I don't mind a jump at the start or end of my block but I don't want to have to search the whole block for one in the middle.

I also wonder if the case you're describing would be done differently if you have access to the ranges library so could filter out those elements before you even make it into the body of your loop (I don't so I can't say from experience).

[sts1skj]

@shaneasd Okay, let me try again:

(start suggested rule 2, revision 2)

Don't avoid using continue.

Reason: The avoid-continue rule usually leads to unnecessary nesting and increased indentation (see rule "F.56: Avoid unnecessary condition nesting"). Avoiding continue can also lead to splitting special-case handling code from special-case testing code, often in subtle but error-prone ways (see rule "ES.??: Try to keep code for handling a special case close to the test for that special case").

Example:

// bad
for (const auto& rec : records)
{
    if (!rec.special_flag_5)
    {
        // fifty lines of code
    }
}

In this example, fifty lines of code are nested and indented unnecessarily, just to avoid using continue. Furthermore, there is effectively an else continue after the if statement; if the reader wants to know what happens when rec.special_flag_5 is true, they have to look past the fifty lines of code in the if statement to try to find the right right brace and look for the corresponding else clause; this is error-prone (which is why we have rule "ES.??: Try to keep code for handling a special case close to the test for that special case"). It's better to be explicit about what happens in this special case up front rather than relying on the absence of an else clause.

Exception: Using continue deep in the middle of a long loop, or nested two or more scopes deep, can be hard to spot, and thus can be error-prone. See "ES.77: Minimize the use of break and continue in loops". The programmer has to rely on their judgment in this case, and decide if it's simpler and more robust to have a continue statement that's deep into the loop or simpler and more robust to nest many lines of code deeply in if and else constructs in order to avoid that deeply nested continue. Or maybe there's a good way to split some of the loop body into a separate function, possibly replacing continue with return.

See also: Rule "ES.??: Try to keep code for handling a special case close to the test for that special case" and rule "F.56: Avoid unnecessary condition nesting".

(end suggested rule 2, revision 2)

I also wonder if the case you're describing would be done differently if you have access to the ranges library so could filter out those elements before you even make it into the body of your loop (I don't so I can't say from experience).

I haven't used the ranges library. I suspect that using it in this example would be messy. But I, too, am curious what it would look like. (At my current job, we're not allowed to use anything newer than C++-2011, and much of our code base still compiles with a C++-1998 compiler. Our updating task is immense, and I want to get the style rules right along the way.)

[jwakely]

I haven't used the ranges library. I suspect that using it in this example would be messy. But I, too, am curious what it would look like.

As suggested, you just filter out the records you don't want to work with:

for (const auto& rec : records | std::views::filter([](const auto& r) { return !r.special_flag_5; }))
{
  // fifty lines of code
}

Or instead of the lambda expression:

for (const auto& rec : records | std::views::filter(std::not_fn(&Record::special_flag_5)))

[hsutter]

Editors call: We agree with code locality. We think ES.77 correctly says to minimize (but not avoid or eliminate) break and continue. Some other rules we have cover code locality sufficiently (such as T.141).