4

I have a piece of code that can fail at many points. With each additional action (that can also fail), the readability worsens. Is there a better way to structure this? Is there a design pattern for this?

int32_t InitializeMyThingy(void) {
  int32_t retcode;

  retcode = DoA();
  if (retcode != 0) return retcode;
  retcode = DoB();
  if (retcode != 0) return retcode;
  retcode = DoC();
  if (retcode != 0) return retcode;
  retcode = DoD();

  return retcode;
}

Alternatively (more readable?):

int32_t InitializeMyThingy(void) {
  int32_t retcode;

  retcode = DoA();
  if (0 == retcode) {
    retcode = DoB();
    if (0 == retcode) {
      retcode = DoC();
      if (0 == retcode) {
        retcode = DoD();
      }
    }
  }

  return retcode;
}
tarabyte
  • 17,837
  • 15
  • 76
  • 117
  • Depending on if you need `retcode` after the nested `if` statements, you might instead do e.g. `if (0 == DoA()) { ... }`. Unfortunately there is no "right" or "wrong" answer to this question, which is why I voted to close this as primarily opinion based. – Some programmer dude May 02 '15 at 21:05
  • Is there a best practice or article on bubbling up errors? Handled at lowest level or highest? – tarabyte May 02 '15 at 21:09
  • Definitely not the second one. Yuck, that arrow. – Quentin May 02 '15 at 21:22
  • @Quentin I don't like either alternative, not even the one I proposed in my comment, and that together with your comment is exactly why I voted to close this question. :) – Some programmer dude May 02 '15 at 21:28
  • 1
    @JoachimPileborg it probably looks much more opinion-based than it really is, but until someone rolls out an exhaustive list of error handling best practices that no crowd will hate on... Yeah. – Quentin May 02 '15 at 21:35

4 Answers4

5

In C, a SESE (single entry, single exit) pattern with goto is usually appropriate:

int32_t InitializeMyThingy(void) {
  int32_t retcode;

  retcode = DoA();
  if (retcode != 0) goto exit;
  retcode = DoB();
  if (retcode != 0) goto exit;
  retcode = DoC();
  if (retcode != 0) goto exit;
  retcode = DoD();

exit:
  return retcode;
}

By itself, this isn't any different from your first example that has return statements after each check. But once you your code starts allocating resources, the goto approach is much cleaner:

int32_t InitializeMyThingy(void) {
  int32_t retcode = E_FAIL;
  A* a = NULL;
  B* b = NULL;
  C* c = NULL;

  a = AllocateA();
  if (a == NULL) goto exit;

  b = AllocateB(a);
  if (b == NULL) goto exit;

  c = AllocateC(b);
  if (c == NULL) goto exit;

  retcode = DoSomething(c);

exit:
  free(c);
  free(b);
  free(a);
  return retcode;
}

and now you don't have to worry about figuring out what to clean up at every individual exit point.

Despite what many naysayers say about goto, when used properly, it is the sanest form of error-handling in C.

Regarding readability from adding additional actions, I think separating actions with whitespace helps a lot. Or, if you really want compactness, you could consider doing the action and the error check inline:

if ((retcode = DoA()) != 0) goto exit;
jamesdlin
  • 81,374
  • 13
  • 159
  • 204
  • Except, you know, [goto fail](https://www.imperialviolet.org/2014/02/22/applebug.html). – Sinan Ünür May 02 '15 at 21:52
  • `do .. while(0)` as suggested in http://stackoverflow.com/questions/16397961/significance-of-do-while0 allows you to use `break` instead of `goto exit;`. – Jongware May 02 '15 at 21:56
  • 1
    @Jongware I don't see how that's any improvement. It's still `goto`, just with a different name but packaged in an odd construct. – jamesdlin May 02 '15 at 22:01
  • Well I agree with that, but this must be regarded a Win for the No-Goto crowd. It also relieves you of the burden of remembering which label to use. – Jongware May 02 '15 at 22:03
  • 1
    @SinanÜnür Had that code duplicated `return err` instead of `goto fail`, the problem would have remained exactly the same. That bug is not really about `goto`, and SESE/`goto` isn't going to be a magical panacea for all problems. – jamesdlin May 02 '15 at 22:05
  • @jamesdlin Oh, I agree ... And, I do prefer `goto` over SESE, but, #gotofail itself was significant enough to put in a reminder. – Sinan Ünür May 02 '15 at 22:13
  • 1
    This is the quintessential example of when `goto` is appropriate. `goto` is only evil in languages that have better ways of doing things. – BJ Myers May 02 '15 at 23:13
0

I tend to prefer something like this when it is simple error checking without the need for elses.

int32_t InitializeMyThingy(void) 
{
    int32_t           retcode = DoA();
    if (0 == retcode) retcode = DoB();
    if (0 == retcode) retcode = DoC();
    if (0 == retcode) retcode = DoD();

    return retcode;
}
EvilTeach
  • 28,120
  • 21
  • 85
  • 141
-2

That's why other languages have try/catch. Unfortunately, C doesn't have that feature, so your format (I personally prefer the first one) is probably the best way.

You can also take a look at Try catch statements in C.

Community
  • 1
  • 1
clum
  • 479
  • 4
  • 21
-2

There is no particular pattern for this but I use the following format for increased readability and to avoid goto statements.

#define BREAK_ON_ERROR(ret_val)  { if (0 != ret_val) break; }

int32_t InitializeMyThingy(void)
{
    int32_t ret = 0;

    do{
        ret = DoA(); BREAK_ON_ERROR(ret) ;
        ret = DoB(); BREAK_ON_ERROR(ret) ;
        ret = DoC(); BREAK_ON_ERROR(ret) ;
        ret = DoD(); BREAK_ON_ERROR(ret) ;

        /* 
             Code executed Gracefully.
             Do Successful handling here. 
        */
        return ret;
    }while(0);

    /* 
         Error Occurred.
         Handle Error Here.
    */
    return ret;
}
Kanwar Saad
  • 2,138
  • 2
  • 21
  • 27
  • 1
    [jamesdlin](http://stackoverflow.com/users/179715/jamesdlin)'s [goto example](http://stackoverflow.com/a/30007860/369450) is much clearer than this. If it (`do {...} while(0)`) looks like a duck (`goto`) and quacks like a duck (`break` to simulate `goto`), then it's a duck (`goto`). – Uyghur Lives Matter May 03 '15 at 01:24