3

I'd like to see if there has been any precedent on alternatives to nested-ifs--particularly for error-code returns. My workplace requires one return per function, so I cannot early exit. Here is some sample code, rewritten to protect confidentiality:

static ErrorCode_t FooBar ( Input_t input ) {
    ErrorCode_t errorCode = FAIL;

    if ( condition1IsNotMet )
    {
        errorCode = ERROR_CODE_1;
    }
    else
    {
        doSomethingHere;

        if ( condition2IsNotMet )
        {
            errorCode = ERROR_CODE_2;
        }
        else
        {
            doAnotherThingHere;
        }
    }

    return errorCode;
}

I would have like to show my attempt toward writing an alternative to this; however, I can't think of any else frankly. I cannot do chained if, else-if, else-if, etc. because procedures must execute between condition checking.

Any suggestions would be appreciated.

Edit: Please note my requirements before marking as duplicate. I need to have a single return. I need to have behavior occur between condition checking. Also note the rationale behind a single return (though I do not agree) is for readability, not performance.

Saxpy
  • 41
  • 6
  • 4
    This is one of the bad things about single return. And I assume using the dreaded `goto` is completely out of the question? – 1201ProgramAlarm Nov 14 '19 at 02:00
  • 3
    Early returns are fine if you're not using malloc in your functions. – Robert Harvey Nov 14 '19 at 04:23
  • 4
    Possible duplicate of [Elegant ways to handle if(if else) else](https://softwareengineering.stackexchange.com/questions/122485/elegant-ways-to-handle-ifif-else-else) – gnat Nov 14 '19 at 06:23
  • 5
    *"My workplace requires one return per function"* - then your team has either to change that outdated rule or to live with that nonsense. – Doc Brown Nov 14 '19 at 06:39
  • If your employer enforces an arbitrary rule that is causing an obstacle, then it's up to your employer to decide whether they agree to suspend the arbitrary rule, or accept the obstacle's existence (as a consequence of the arbitrary rule). We can't make or break company policy for you. – Flater Nov 14 '19 at 16:00
  • 1
    @1201ProgramAlarm: I would consider `goto` a much bigger red flag than the single return principle, even though I don't like either. – Flater Nov 14 '19 at 16:01
  • @RobertHarvey I am using something similar to malloc unfortunately. – Saxpy Nov 14 '19 at 18:34
  • @Flater To clarify I didn't really mean to ask for suggestions on how to ask my company to break the rule, I meant more of how I could potentially get around the issue in a cleaner way while following company rules. – Saxpy Nov 14 '19 at 18:35
  • @1201ProgramAlarm I think a goto cleanup could potentially be a candidate, but since I am new I've avoided using it since I am not certain about my company's policy on that. – Saxpy Nov 14 '19 at 18:36
  • 1
    @DocBrown (and @Flater): note that C tag in the question, which means that [it is not an arbitrary/outdated rule](https://softwareengineering.stackexchange.com/a/118717/6605) it would be if we were talking about C# or Python or similar languages. – Arseni Mourzenko Nov 14 '19 at 20:03
  • I would add the fact that it is a C Programming Language question in the answer (clearly stated - not only in the tag). For instance, an answer that involves saying that Exceptions should be thrown is not a valid one (I almost did that). – ianmandarini Nov 14 '19 at 23:47

4 Answers4

6

Ah, Golden Rules of thumb that are worth too much to cut off.

Might I recommend taking whomever is enforcing that rule and giving them this piece of code to improve. Bring pop corn.


What you are writing is not a mathematical formula with a pithy implementation with a single line of logical execution.

What you are writing is a command. To follow the golden rule to its ultimate end will require serious refactoring.

static ErrorCode_t FooBar_impl_2( Input_t input )
{
    doAnotherThingHere;
    return FAIL;
}
static ErrorCode_t FooBar_impl_1( Input_t input )
{
    doSomethingHere;

    return ( condition2IsNotMet )
        ? ERROR_CODE_2
        : FooBar_impl_2(input)
        ;
}
static ErrorCode_t FooBar ( Input_t input )
{
    return ( condition1IsNotMet )
        ? ERROR_CODE_1
        : FooBar_impl_1(input )
        ;
}

Now understand that...

Which is precisely how the optimising compiler is interpreting your example program.

Chalk 1 for the golden rule of single return, you the programmer are the compiler!


BUT.... I'm being disingenuous...

The golden rule of having a single return was coined in the days of assembler (or right now if you are writing in assembler). Back then a function, cough, cough sorry those didn't exist yet...

What I meant was that back then the section of assembler code whose first instruction was labelled with either a line number, or a shiny label could do pretty much whatever it pleased.

When you "called" - I mean jumped to - that instruction it did not have to return back to whomever "called" - I mean jumped to - it.

The convention of the time was to place the "return address" in a special register, or on the stack at a specific offset from the current top of the stack. The idea being that the jumped to set of instructions would do what it did, and jump back to that address.

Guess what smart programmers didn't do?

And that is how the rule of having a single return came to be. The section of jumped to code, must return back to the address it was given, and nowhere else.

Any modern function does exactly that. Whomever called it will be returned to when the function is done doing whatever it does (whether or not it actually finishes what it does is beside the point).

With that in mind, I would refactor as:

static ErrorCode_t FooBar ( Input_t input )
{
    if ( condition1IsNotMet )
        return ERROR_CODE_1;

    doSomethingHere;

    if ( condition2IsNotMet )
        return ERROR_CODE_2;

    doAnotherThingHere;

    return FAIL;
}

And the compiler guarantees the rule of thumb.

Kain0_0
  • 15,888
  • 16
  • 37
  • This is a good answer (+1), but to be fair, the "single exit" rule isn't exclusively a relict from assembler. It makes some sense in C when a function allocates some resources (like memory, using `malloc`) which must be deallocated before the function ends. Otherwise, deallocation code must be duplicated at multiple places (before each return) inside the function, and it becomes pretty error prone to extend this kind of function afterwards. However, the OP's example does not look like one where resources were allocated. – Doc Brown Nov 14 '19 at 11:52
  • +1 for the rationale, but my workplace explicitly requires single returns for readability rather than for older link register reasons. An edit to the question was made above. – Saxpy Nov 14 '19 at 18:44
  • 1
    Fair enough, I can certainly understand using structured programming to improve code readability, but that is a far cry from having a single return point. Also @DocBrown has a very good point that managing resources in an unmanaged environment can be made much simpler by using a single point of return. I still wouldn't make it mandatory. At worst a code review with a decent reasoning as to why not. – Kain0_0 Nov 17 '19 at 22:54
3

I vote for multiple returns, but the following pattern sometimes fits. It assumes that a null (or 0) "error" means "all is well.

static ErrorCode_t FooBar ( Input_t input ) {
  ErrorCode_t errorCode = null;

  if ( condition1IsNotMet )
  {
    errorCode = ERROR_CODE_1;
  }

  if (!errorCode) {
    doSomethingHere;

    if ( condition2IsNotMet )
    {
        errorCode = ERROR_CODE_2;
    }
  }

  if (!errorCode) {
    doAnotherThingHere;
  }

  return errorCode;
}

(Added much later) Here is how the code might look if you broke it up into several smaller functions, each of which returns an ErrorCode_t.

static ErrorCode_t FooBar ( Input_t input ) {

  ErrorCode_t errorCode = checkPreconditions(input);

  if (!errorCode) {
    doSomethingHere();
    errorCode = checkCondition2();
  }

  if (!errorCode) {
    errorCode  = doAnotherThing();
  }

  return errorCode;
}
user949300
  • 8,679
  • 2
  • 26
  • 35
  • Between Kain0_0's answer and this one, it's purely a matter of taste. And both are equally superior to the big nested if (which I have seen 12 levels deep nested, each with an else statement about two meters away from the if). And both superior to writing dozens of little functions. – gnasher729 Nov 14 '19 at 17:44
  • Actually I think this answers my question better because I am required to follow single return patterns. It's not my decision to change company policy. That being said, there is a chance on a really bad compiler that conditional checking will occur through each phase. As I am developing firmware that might not fly with code review, but I like this the best so I'll give it a try. – Saxpy Nov 14 '19 at 18:47
  • 1
    Even though minimizing the number of functions is something to aim, It is arguable that correctly expressing design ideas is much more important. I do not agree that his solution is "better than writing dozens of little functions". The `FooBar` function in this answer seems to have more than one responsibility, judging by the number of "sections" it has. – ianmandarini Nov 14 '19 at 23:28
  • 1
    I agree with @Albuquerque that splitting this into smaller function, each of which returns a possible error code, is a likely improvement. – user949300 Nov 15 '19 at 05:14
2

You are asking the wrong question

You have asked about alternatives for your nested-ifs but I believe this is a case of a XY Question.

The XY problem is asking about your attempted solution rather than your actual problem.

That is, you are trying to solve problem X, and you think solution Y would work, but instead of asking about X when you run into trouble, you ask about Y.

I believe that the true question you are facing is one about clean code/ code quality.

Asking how to rewrite nested-ifs is just your current attempt at trying to clean the FooBar function. As a result, I will attempt to answer the following question:

How can I improve the code quality of my FooBar function?

For my answers, I will be referencing and quoting the brilliant must-read book by Robert C. Martin Clean Code: A Handbook of Agile Software Craftsmanship


Your functions should do one thing

Take a look at your example. Does it look that it does only one thing?

static ErrorCode_t FooBar (Input_t input)
{
    ErrorCode_t errorCode = FAIL;
    if (condition1IsNotMet) {
        errorCode = ERROR_CODE_1;
    } else {
        // doSomethingHere
        if (condition2IsNotMet)
            errorCode = ERROR_CODE_2;
        else
            // doAnotherThingHere
    }

    return errorCode;
}

It doesn't.

And the various if-elses dividing the code into big chunks is a good indication of so. Your code clearly has sections, making them inherently more complex to reason.

Functions should do one things. They should do it well. They should do it only.


Your functions should be small

Functions should Hardly be 20 lines long.

in fact, most functions could be expected to be less than 10 or even less than 5 lines long. Wait! How am I going to write a nested-if with so few lines? Well, Robert C. Martin writes:

(...) blocks within if statements, else statements, while statements, and so on should be one line long. Probably that line should be a function call. Not only does this keep the enclosing function small, but it also adds documentary value because the function called withing the block can have a nicely descriptive name.

This also implies that functions should not be large enough to hold nested structures. (...)


Refactoring your code

Using these two principles, it should be clear that the doSomethingHere and doAnotherThingHere should be independent functions and that we need to break your nested if. It would end up looking something like this:

static ErrorCode_t FooBar(Input_t input)
{
    if (condition1IsNotMet)
        return ERROR_CODE_1;
    return DoSomethingHere(input);
}

static ErrorCode_t DoSomething(Input_t input)
{
    // doSomethingHere
    return HandleMoreThings(input);
}

static ErrorCode_t HandleMoreThings(Input_t input)
{
    if (condition2IsNotMet)
        return ERROR_CODE_2;
    return DoAnotherThing(input);
}

static ErrorCode_t DoAnotherThing(Input_t input)
{
    // doAnotherThingHere
    return FAIL;
}

How you break up your functions depend of the one thing they do

I can only speculate about the behavior of your code. But now we can ask the questions for each one of the smaller functions:

static ErrorCode_t FooBar(Input_t input);
static ErrorCode_t DoSomething(Input_t input);
static ErrorCode_t HandleMoreThings(Input_t input);
static ErrorCode_t DoAnotherThing(Input_t input);

For each one of them, what is the expected result? The final code may vary according to the actual intent of your code.

For example, you could argue that the code should be written as:

static ErrorCode_t FooBar(Input_t input)
{
    if (condition1IsNotMet)
        return ERROR_CODE_1;

    result = DoSomethingHere(input);

    if(result == SUCCESS);
        return ERROR_CODE_2
    else
        return FAIL;
}

static AnotherErrorCode_t DoSomething(Input_t input)
{
    // doSomethingHere
    return HandleMoreThings(input);
}

static AnotherErrorCode_t HandleMoreThings(Input_t input)
{
    if (condition2IsNotMet)
        return SUCCESS;
    DoAnotherThing(input);
    return FAIL;
}

static void DoAnotherThing(Input_t input)
{
    // doAnotherThingHere
}

I cannot tell without the details.

The one thing your code does cannot be told by only looking at its conditional statements. Hence, you will need to adapt this answer to your code using your best judgment.


A note about the "only one return statement" constraint

In the end of your question, you have added:

Please note my requirements before marking as duplicate. I need to have a single return. I need to have behavior occur between condition checking. Also note the rationale behind a single return (though I do not agree) is for readability, not performance.

By actually dividing your functions in smaller chunks, you get readability without that artificial rule of "having only one return in the end". in fact, that rule seems to be an artificial attempt to solve a "bad code" problem.

Also, even if you have to follow the one return at the end rule (perhaps because of a company guideline), it is much easier to do so if you divide your code into smaller chunks.


Bottom Line

The problem is neither having multiple returns or writing the nested-ifs in a good way.

The problem is that multiple returns indicate that your code does not do one thing only. Same thing for the nested-ifs.

By organizing your code in smaller chunks, you will get a better code.

ianmandarini
  • 2,758
  • 12
  • 26
0

user949300's suggestion is very good, but it assumes that the you want to skip all the "optional" steps as soon as an error is encountered and makes "early success" more cumbersome.

I think those are both valid assumptions nearly all the time, but in case they aren't, here are two ways of skipping some steps independent of the value of the status code you're eventually going to return.

  1. (Ab)use a builtin control-flow structure such as do ... while or switch
  2. Use a keep-going variable, possibly hiding it behind a macro.

Here's an example using a do ... while loop.

// foo_do_while.c
#include <stdio.h>
#include <stdbool.h>

#define CONDITION_1 true
#define CONDITION_2 true

#define ERROR_CODE_INVALID (-1)
#define ERROR_CODE_1 1
#define ERROR_CODE_2 2

void do_something(void) {
    printf("do_something\n");
}

int no_nest(void) {
    int error_code = ERROR_CODE_INVALID;
    do {
        if (CONDITION_1) {
            printf("1\n");
            error_code = ERROR_CODE_1;
            break;
        }
        do_something();
        if (CONDITION_2) {
            printf("2\n");
            error_code = ERROR_CODE_2;
            break;
        }
    } while(0);
    return error_code;
}

int main() {
    printf("no_nest status: %d\n", no_nest());
    return 0;
}

and using a switch statement.

// foo_switch.c
#include <stdio.h>
#include <stdbool.h>

#define CONDITION_1 true
#define CONDITION_2 true

#define ERROR_CODE_INVALID (-1)
#define ERROR_CODE_1 1
#define ERROR_CODE_2 2

void do_something(void) {
    printf("do_something\n");
}

int no_nest(void) {
    int error_code = ERROR_CODE_INVALID;
    switch (0) {
    case 0:
        if (CONDITION_1) {
            printf("1\n");
            error_code = ERROR_CODE_1;
            break;
        }
        do_something();
        if (CONDITION_2) {
            printf("2\n");
            error_code = ERROR_CODE_2;
            break;
        }
    }
    return error_code;
}

int main() {
    printf("no_nest status: %d\n", no_nest());
    return 0;
}

Here's using a keep-going variable explicitly.

#include <stdio.h>
#include <stdbool.h>

#define CONDITION_1 false
#define CONDITION_2 true

#define ERROR_CODE_INVALID (-1)
#define ERROR_CODE_1 1
#define ERROR_CODE_2 2

void do_something(void) {
    printf("do_something\n");
}

int no_nest(void) {
    bool keep_going = 1;
    int error_code = ERROR_CODE_INVALID;
    if (keep_going) {
        if (CONDITION_1) {
            printf("1\n");
            error_code = ERROR_CODE_1;
            keep_going = 0;
        }
    }
    if (keep_going) {
        do_something();
    }
    if (keep_going) {
        if (CONDITION_2) {
            printf("2\n");
            error_code = ERROR_CODE_2;
            keep_going = 0;
        }
    }
    return error_code;
}

int main() {
    printf("no_nest status: %d\n", no_nest());
    return 0;
}

It's also possible to hide the keep_going check behind a macro, assuming that macros are not prohibited by the same standards that prohibit multiple returns. This example shows a macro taking a block and a begin/end pair of macros.

#include <stdio.h>
#include <stdbool.h>

#define CONDITION_1 false
#define CONDITION_2 true

#define ERROR_CODE_INVALID (-1)
#define ERROR_CODE_1 1
#define ERROR_CODE_2 2


#define CHK_KG(block) \
    do { \
        if (keep_going) { \
            block \
        } \
    } while (0);

#define BEGIN_CHK_KG \
    do { if (keep_going) {   {{{{{{{{{{

#define END_CHK_KG \
    }}}}}}}}}} } } while(0);

void do_something(void) {
    printf("do_something\n");
}

int no_nest(void) {
    bool keep_going = 1;
    int error_code = ERROR_CODE_INVALID;
    CHK_KG(
        if (CONDITION_1) {
            printf("1\n");
            error_code = ERROR_CODE_1;
            keep_going = 0;
        }
    )
    BEGIN_CHK_KG
    do_something();
    END_CHK_KG
    if (keep_going) {
        if (CONDITION_2) {
            printf("2\n");
            error_code = ERROR_CODE_2;
            keep_going = 0;
        }
    }
    return error_code;
}

int main() {
    printf("no_nest status: %d\n", no_nest());
    return 0;
}
Greg Nisbet
  • 288
  • 1
  • 9