1

Currently I am using gotos for closing handles and general cleanup if an error occurs.
Since I don't want to nest all the ifs and a function for cleanup would require a return besides the cleanup(); call, I don't have any good ideas left.

The structure is like so:

if (condition1) {
   goto cleanup;
}

if (condition2) {
   goto cleanup;
}

if (condition3) {
   goto cleanup;
}

// do stuff only if all checks passed

cleanup:
CloseHandle(x);
CloseHandle(y);
// etc.
Benjoyo
  • 162

3 Answers3

7

The cleanup could be in an outer function, and then return can be used instead of goto:

void main_func() {
    /* Set-up goes here */
    handle x = ...;
    handle y = ...;
    void result = inner_func(x, y);

    /* Clean-up goes here */
    CloseHandle(x);
    CloseHandle(y);
}

void inner_func(x, y) {
    if (condition1) return;
    if (condition2) return;
    if (condition3) return;

    /* Do things here */
}

But goto for things like this isn't that bad, in my opinion.

RemcoGerlich
  • 3,330
  • 20
  • 24
4

In C a typical way to simplify error checking and avoid deep nested if is:

do
{
  if (condition1) break;

  /* 1. do something... */

  if (condition2) break;

  /* 2. do something else... */

  if (condition3) break;

  /* 3. do something else... */
} while(0);

/* Cleanup */

There are various opinion on this "idiom" (e.g. take a look at Do you consider this technique “BAD”?).

Possibly you should add a comment on the do to make it clear to anyone what's happening.

You can often rewrite the code using a helper function and change the fake do-loop into:

void func(X *x, Y *y)
{
  if (condition1) return;

  /* 1. do something... */

  if (condition2) return;

  /* 2. do something else... */

  if (condition3) return;

  /* 3. do something else... */
}


/* ... */
X x;
Y y;

func(&x, &y);

/* Cleanup */

and it won't be considered a "bad practice" since it's more expected.

If you haven't the intermediate steps (1. and 2.) this is probably enough:

int stop = condition1 || condition2 || condition3;

if (!stop)
{
  /* ... */
}

/* Cleanup */
Community
  • 1
manlio
  • 4,256
4

You could try saying what you mean:

if (!condition1
 && !condition2
 && !condition3 ) {

// do stuff only if all checks passed
}
CloseHandle(x);
CloseHandle(y);
// etc.

As various commenters have pointed out this is only readable/maintaibable if the condition tests are fairly simple where complex conditions are involved something like this is cleaner:-

if (doAble()) {
   // usefull work here
}
CloseHandle(x);
CloseHandle(y);

boolean doAble() {
  if (condition1) {
     return false;
  }
  if (condition2) {
     return false;
  }
  if (condition3) {
     return false;
  }
  return true;
}

However if you need lots of local variables to evaluate the conditions this too can get messy.

James Anderson
  • 18,255