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.
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.
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
goto is that bad.
– Konamiman
Mar 24 '15 at 14:48
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
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
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 */
/* do something else */ part, right? And what's wrong with putting the stuff in a function?
– RemcoGerlich
Mar 24 '15 at 12:15
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.
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 thegotostatement. 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:21gotoin 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:20gototo implement a custom control structure. Go recognized this for what it is - a control structure - and added thedeferstatement to cover this (valid) use-case. – Riking Mar 24 '15 at 17:59