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
  • 1
  • 8
  • ...see also: [Is this a decent use-case for goto in C?](http://programmers.stackexchange.com/questions/154974/is-this-a-decent-use-case-for-goto-in-c) – gnat Mar 24 '15 at 12:08
  • 12
    Just use `goto`. It makes the intent of the code clear and is cleaner than camouflaging the control flow by any misguided attempt to cheat around using the `goto` statement. I really dislike the idea of blindly following “never do X” rules when X actually is the best (least bad) alternative in a given case. This particular pattern (`goto cleanup;`) is actually quite idiomatic in C. – 5gon12eder Mar 24 '15 at 12:21
  • 2
    There was a [study](https://peerj.com/preprints/826v1/) (pre-print, not peer-reviewed) on the use of `goto` in C that concluded that this is the case where they're being used and that applications which cause spaghetti are relatively rare. – Blrfl Mar 24 '15 at 16:20
  • 1
    This is basically using `goto` to implement a custom control structure. Go recognized this for what it is - a control structure - and added the `defer` statement to cover this (valid) use-case. – Riking Mar 24 '15 at 17:59
  • @5gon12eder, not that I don't want to use goto, just my boss doesn't want me use it. – Zhang Dec 29 '20 at 13:38

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,280
  • 18
  • 22
  • ok, that could actually be a possibility. – Benjoyo Mar 24 '15 at 12:24
  • The slight problem I have with this is the same that I have with manlio's third solution. – Benjoyo Mar 24 '15 at 12:36
  • wouldn't this increase the coupling between the two part of the code? Any coder calling `inner_func` would have to first open the files and then remember to close them. code reviews would be ineffective here to catch the problem. – Kinjal Dixit Mar 24 '15 at 13:23
  • 1
    @KinjalDixit: the function wouldn't be meant to be called from elsewhere. My C is too rusty to suggest how to handle that -- maybe start the name with an underscore, don't put it in the .h file, et cetera. – RemcoGerlich Mar 24 '15 at 13:26
  • +1, this is the best solution. But I have to disagree in the last sentence: given a perfectly valid and readable alternative, `goto` _is_ that bad. – Konamiman Mar 24 '15 at 14:48
  • 1
    @Konamiman but it isn't as good as the goto solution, since you shouldn't call the other functions when one before failed. – Benjoyo Mar 24 '15 at 15:51
  • 1
    @RemcoGerlich: You're looking for `static` to make the function invisible to the outside. A good compiler would take care of inlining it, which would result in the same code as a `goto` without the overhead of a function call. – Blrfl Mar 24 '15 at 16:23
  • Just a reminder, handle `x` may succeed while handle `y` may fail. Or vice versa. One will need to check the validity of each handle before calling `CloseHandle`. In situations where if `x` fails and one wants to bail out without trying to acquire `y`, one will still have to hand-code the "variable unwind" code - something C++ can automatically generate, but C programmers must write manually. – rwong Mar 24 '15 at 17:07
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,166
  • 3
  • 23
  • 35
  • Good idea, but this would require a function or goto for the things to be done if all checks passed. – Benjoyo Mar 24 '15 at 12:12
  • @Benjoyo: that other stuff could go in the `/* do something else */` part, right? And what's wrong with putting the stuff in a function? – RemcoGerlich Mar 24 '15 at 12:15
  • @RemcoGerlich I would need to pass a ton of handles and so on. Also this isn't really such a good SoC that I would like to use a function. – Benjoyo Mar 24 '15 at 12:27
  • Your third suggestion is an option, I didn't consider something like this because I didn't want to, if the first function fails, force the following ones to try their luck even if it's clear that they will fail. – Benjoyo Mar 24 '15 at 12:33
  • The first idea abuses the while/break language feature, goto can be considered more explicit and cleaner in such a case. – johannes Mar 24 '15 at 15:53
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,049
  • 1
  • 42
  • 72
  • Wow, saying what you mean is also simpler and clearer than the alternatives. Who would have thunk it? – user949300 Mar 24 '15 at 17:27
  • Maybe this is really a good option, since evaluation will stop if one condition is false, right? – Benjoyo Mar 24 '15 at 17:39
  • Which also gets you a hard-to-reason about chain of negatives, nowhere to put any condition-specific logic or logging, and some free indentation. It's fine in the case shown, but IMO scales poorly as the number (or complexity) of conditions grows. – Useless Mar 24 '15 at 18:53
  • If the number or complexity of conditions grows, it should be broken into smaller pieces and separate functions. – Rob K Mar 24 '15 at 20:57