More Return Statements vs. More Indentation

Question

Which of the following two blocks of code is better overall?

One return statement, more indented code:

struct dnode *dllist_push_front(struct dllist *dllist, void *data)
{       
        struct dnode *dnode = malloc(sizeof(struct dnode));
        if (dnode) {
                dnode->data = data;
                dnode->next = dllist->sentinel->next;
                dnode->prev = dlllist->sentinel;
                dnode->next->prev = dnode;
                dllist->sentinel->next = dnode;
                dllist->size++;
        }
        return dnode;
}

or,

Two return statments, less indented code:

struct dnode *dllist_push_front(struct dllist *dllist, void *data)
{
        struct dnode *dnode = malloc (sizeof(struct dnode));
        if (!dnode)
                return NULL;
        dnode->data = data;
        dnode->next = dllist->sentinel->next;
        dnode->prev = dlllist->sentinel;
        dnode->next->prev = dnode;
        dllist->sentinel->next = dnode;
        dllist->size++;
        return dnode;
}

You have two return statements in the top code segment as well. — 1'', Aug 12 '13 at 00:50
Another effect of the 2 approaches is the negation (or not) in the `if()`. There is a subtle advantage in `if()` that are without negation. `if (working()) ... if (failed()) ...` vs. `if (!working()) ... if (!failed()) ...`. Not that it trumps your question, but a consideration. Want to avoid `if()` that are like court rulings "Supreme Court overturns order to delete negative reviews" — chux - Reinstate Monica, Aug 12 '13 at 01:16
At the same time, the more code there is within the if statement, the more vertical space you take up. If it is an unusually long block of code, scrolling just to see the function do nothing in the case of `!dnode` would be annoying. For the sakes of simplicity and readability, I'd argue in favor of the "fail fast" approach and go with the multiple `return` statements. Both `!` and `NULL` tend to get my attention too, further helping me understand the code, but maybe that is just me. — , Aug 12 '13 at 02:05
I thought it was a pretty common C idiom to check for NULL pointers without a logical comparison, however, is it possible that comparing with NULL would be more portable? I don't think it would be, but maybe it is. Or is it non-standard?... — oddlogic, Aug 12 '13 at 05:02

paxdiablo · Answer 1 · 2013-08-12T01:32:03.583

You can actually go either way though I tend to prefer the latter since it immediately calls out the "failure" condition. My personal preference would be to have a blank line following the first return since it calls out that condition even more (and I'm a big fan of comments):

struct dnode *dllist_push_front(struct dllist *dllist, void *data) {
    // Get new node, exit immediately if not possible.

    struct dnode *dnode = malloc (sizeof (struct dnode));
    if (!dnode) return NULL;

    // Node exists here, populate data and pointers then return it to caller.

    dnode->data = data;
    :
    return dnode;
}

Keep in mind that multiple return points can be bad but it's not necessarily always the case.

You have to remember the reasons behind the guidelines and the reason why multiple return points are bad is because they can sometimes lead to code that's hard to understand.

If you can see the flow easily (and you can in both your cases unless your editor window is less than 13 lines high), multiple return points are just fine.

Sorry, I didn't mean to do this, but I had an extra return statement on the top block of code. I changed it to my intention and it matches your example. :/ Thank you for the reply. — oddlogic, Aug 12 '13 at 01:01
@oddlogic, no probs, have adjusted the answer to suit your question edits. — paxdiablo, Aug 12 '13 at 01:02
Thanks for your input. I like the idea of putting the return statement on the same line as the test condition. — oddlogic, Aug 12 '13 at 01:12

score 1 · Answer 2 · edited May 23 '17 at 12:21

1

I would err on the side of "fail fast", meaning don't wait longer than necessary if you hit some condition that's going to break your program. Otherwise, your code can start turning into an unwieldy mess of nested if clauses.

See: Should a function have only one return statement?

edited May 23 '17 at 12:21

Community

1
1

answered Aug 12 '13 at 00:57

jslivka

361
3
7

1

In this example. though, they both fail at the same time under the same conditions... – oddlogic Aug 12 '13 at 01:02
I was linking to the thread itself and not any particular answer. – jslivka Aug 12 '13 at 01:20
I am referring to my example. Sorry the confusion. – oddlogic Aug 12 '13 at 01:29

alk · Answer 3 · 2013-08-12T09:51:11.567

For the sake of clear flow and debugging have each function have exactly one exit point.

Making disciplined use of goto makes things easy:

#include <stdio.h>
#include <stdlib.h>

...

typedef enum 
{
  Error_Uninitialised = -1,
  Error_Ok = 0,
  Error_1,
  Error_2,
  Error_3,
  Error_4,
  Error_5,
  ...
} ErrorCode_t;

#ifdef DEBUG
#define LOG(fmt, ...) fprintf(stderr, fmt, __VA_ARGS__)
#else
#define LOG(fmt, ...)
#endif

int func(int a, char b, void * p, ...)
{
  LOG("entering: '%s'\n", __func__);

  int result = Error_NoError;

  if (42 == i)
  {
    result = Error_1;
    goto lblExit;
  }

  if ('y' == c)
  {
    result = Error_2;
    goto lblExit;
  }

  if (!p)
  {
    result = Error_3;
    goto lblExit;
  }

  void * ptmp = realloc(p, i);
  if (!ptmp)
  {
    result = Error_4;
    goto lblExt;
  }

  ...

  lblExit:

  LOG("'%s' returns %d\n", __func__, result);

  return result;
}

For those who restrict themselfs from using goto the following version might help:

int func(int a, char b, void * p, ...)
{
  LOG("entering: '%s'\n", __func__);

  int result = Error_NoError;

  do
  {
    if (42 == i)
    {
      result = Error_1;
      break;
    }

    if ('y' == c)
    {
      result = Error_2;
      break;
    }

    if (!p)
    {
      result = Error_3;
      break;
    }

    void * ptmp = realloc(p, i);
    if (!ptmp)
    {
      result = Error_4;
      break;
    }

    ...

  } while (0);

  LOG("'%s' returns %d\n", __func__, result);

  return result;
}

If then later during the project, when it comes to optimisation one might step away from this approach if really necessary.

Well, I don't find an 'early return', eg. on an error condition or sucessfull match during the search of a long list, to be any particular barrier to debugging, especially with optimizations off an with the return() on a line of its own. — Martin James, Aug 12 '13 at 09:46
I would expect that someone open to the disciplined use of goto wouldn't be so adverse to the disciplined use of multiple return points :-) Blind application of these "rules" without thinking often leads to code that is _much harder_ to follow. — paxdiablo, Aug 12 '13 at 21:50

More Return Statements vs. More Indentation

3 Answers3