Blocks and Their Use

[Army1987]

I think this is better handled as:

if (error) ret_val = FAIL;
else {
ret_val = success;
/* lotsa code */
/* which may include "ret_val = FAIL\" overides */
}
return ret_val;

Which incorporates several guidelines. One is single point of
exit.

In such cases, why

if (error) return FAIL;
/* lotsa code */
return SUCCESS; /*or anything else*/

is so bad? (I know that there are people who don't like writing "return"
(quotation marks excluded) more than once within a function body (even if I
disagree, I can see the point of that, in the case of "normal" working),
but, in the case of error handling, I can't see how a premature return is
worse than having all the function's normal operation within an else block.)

 

[CBFalconer]

In such cases, why

if (error) return FAIL;
/* lotsa code */
return SUCCESS; /*or anything else*/

is so bad? (I know that there are people who don't like writing
"return" (quotation marks excluded) more than once within a
function body (even if I disagree, I can see the point of that,
in the case of "normal" working), but, in the case of error
handling, I can't see how a premature return is worse than
having all the function's normal operation within an else block.)

Did I say it was totally evil? At any rate, for one thing while
reading "lotsa code" you can't inherently see that execution
depends on passing the error test. For another, it is much easier
to trap the return point.

 

[Richard Heathfield]

Army1987 said:

[...] why

if (error) return FAIL;
/* lotsa code */
return SUCCESS; /*or anything else*/

is so bad?

Well, it isn't really so terribly bad.

(I know that there are people who don't like writing
"return" (quotation marks excluded) more than once within a function
body (even if I disagree, I can see the point of that, in the case of
"normal" working), but, in the case of error handling, I can't see how
a premature return is worse than having all the function's normal
operation within an else block.)

Well, you've put your finger on something rather important here - the
concept of "normal working". When you're just bailing out because of an
error that means you can't possibly go on, then (as long as you've
cleaned up after yourself, so to speak) an early return is no big deal,
and the only reason one might want to avoid it is mere style, in that
it simplifies control flow if everything aims towards one exit point
and therefore some people find it easier to follow the code.

But where it really, really matters is in code like this:

double ctx2174_frob_foo(foo *f, bar *b, baz *z, quux *q)
{
int i, j, k, l, m, n, o, p;
double foo_rate = 1.0;
double d = get_init_d_from_db(f->red, b->arney);

if(d < 0)
{
push_error(__FILE__, __LINE__, "ctx2174_frob_foo", "bad d");
return d;
}

for(i = b->a->barracus->lowentry;
i < q->u->ality->street;
i += z->e->bedee)
{
j = f->rank + b->ill->bob;
d += b->ubble * f->roth[j / 2] * q->uestion;
foo_rate *= d / b->arrack->room[j];
if(z->ygote > foo_rate)
{
for(k = 0; k / j > f->eed->the->world; k += i)
{
if(k + 4 < 3 / j)
{
push_error(__FILE__, __LINE__,
"ctx2174_frob_foo", "bad k/j ratio");
return d * j;
}
if(k * j > foo_rate / 3.14159)
{
f->lipside->folksy *= get_mod_x_from_db(z->ymurgy, j);
if(f->lipside->folksy / d < b->arrack->room / 9)
{
i++;
continue;
}
f->rank += 2;
if(f->rank < b->ill->b->ill)
{
z->o->o->keeper += d;
return foo_rate;
}
}
}
if(d * f->rank < b->alder->dash && f->orte < b->attledress)
{
return foo_rate / d;
}
}
for(l = 0, m = z->yx;
l < m;
l += b->ill->joe, m += b->ill->bob)
{
d += z->ygote * f->lipside->folksy[l];
if(get_mod_x_from_db(z->ymurgy, l) > m)
{
f->lipside->rocky[l] = get_mod_x_from_db(z->ymurgy, m);
return foo_rate * f->eed->the->fish;
}
else if(f->antasy->foot->ball < q->ual->ityof->life)
{
f->lipside->rap = get_mod_y_from_db(z->yzygy, m - l);
break;
}
else if(f->orei->gnle->gion < b->ill->jean)
{
f->lipside->country = get_mod_x_from_db(z->orch, l + --m);
}
foo_rate += f->ast->andl->oose[l];
do
{
n = 17 * q->uinine[m] *
get_magic_number((b->ilbo + f->rodo) / 2);
if(n < rune_caster(i, m))
{
return f->eed->the->tor->toise * 6;
}
} while(n > m);
}
}

for(i = ....

etc etc. Imagine another ten or twelve similar loops, each packed to the
gunwhales with other loops, each seemingly random in structure and yet
crammed with business logic (and if ever there were an oxymoron
contest, "business logic" would definitely be my entry), with returns -
some of them seemingly unrelated in nature to the others - scattered
liberally through the loops, and a final

return foo_rate + q->uive->ring->wrec[k];
}

to wind things up neatly at the end.

That is the kind of code that put me off multiple returns (and,
incidentally, also turned me off break and continue as loop-diddling
constructs).

Curiously, of all the industries I've worked in, insurance seems to be
the biggest culprit for this kind of code. I don't know what it is
about insurance, but second place definitely goes to banks, which might
be a clue of some kind.

Perhaps those who share my view about multiple returns are those who
have spent at least some of their career in the banking/insurance
world, and those who don't aren't? Just a thought.

 

[Ian Collins]

Perhaps those who share my view about multiple returns are those who
have spent at least some of their career in the banking/insurance
world, and those who don't aren't? Just a thought.

In my case, the allergy was cased by trying to fix up smelly embedded
code with multiple returns that failed to clean up after its self.

 

[jaysome]

Army1987 said:

[...] why

if (error) return FAIL;
/* lotsa code */
return SUCCESS; /*or anything else*/

is so bad?

Well, it isn't really so terribly bad.

(I know that there are people who don't like writing
"return" (quotation marks excluded) more than once within a function
body (even if I disagree, I can see the point of that, in the case of
"normal" working), but, in the case of error handling, I can't see how
a premature return is worse than having all the function's normal
operation within an else block.)

Well, you've put your finger on something rather important here - the
concept of "normal working". When you're just bailing out because of an
error that means you can't possibly go on, then (as long as you've
cleaned up after yourself, so to speak) an early return is no big deal,
and the only reason one might want to avoid it is mere style, in that
it simplifies control flow if everything aims towards one exit point
and therefore some people find it easier to follow the code.

But where it really, really matters is in code like this:

double ctx2174_frob_foo(foo *f, bar *b, baz *z, quux *q)
{
int i, j, k, l, m, n, o, p;
double foo_rate = 1.0;
double d = get_init_d_from_db(f->red, b->arney);

if(d < 0)
{
push_error(__FILE__, __LINE__, "ctx2174_frob_foo", "bad d");
return d;
}

for(i = b->a->barracus->lowentry;
i < q->u->ality->street;
i += z->e->bedee)
{
j = f->rank + b->ill->bob;
d += b->ubble * f->roth[j / 2] * q->uestion;
foo_rate *= d / b->arrack->room[j];
if(z->ygote > foo_rate)
{
for(k = 0; k / j > f->eed->the->world; k += i)
{
if(k + 4 < 3 / j)
{
push_error(__FILE__, __LINE__,
"ctx2174_frob_foo", "bad k/j ratio");
return d * j;

Here we mask the programmer's error and merrily trudge along in the
hopes that returning d * j somehow makes things okay. Happy, happy,
happy!

What exactly does push_error() do? Log an error message to a file that
the user is supposed to monitor on a daily (or hourly) basis? And if
there are any entries in this error log, should the user get on the
phone to the programmer and report that there's something "wrong"
going on? And should the user, despite these errors, feel comfortable
in knowing that the program is still running?

More likely than not, the program will exhibit some sort of undefined
behavior. It would be better, IMHO, to change that to:

assert(k + 4 >= 3 / j);

The big fat error message you get when running your test cases and the
assert that fires off, or worse yet when your users get that same, big
fat error message when running your program and the assert fires off,
nips the problem in the bud and allows you to ascertain what is the
real problem. If you're really good, you'd define your own macro, and
use it instead of assert, e.g.:

MY_ASSERT(k + 4 >= 3 / j);

MY_ASSERT dumps all sorts of information that allows you to track down
*your* bug--hopefully at verification time! MY_ASSERT could even do
the duty of push_error *and* restart the program, with no less down
time than what the requirements specify.

Best regards

 

[Richard Heathfield]

jaysome said:

On Fri, 30 Mar 2007 07:43:05 +0000, Richard Heathfield



Here we mask the programmer's error and merrily trudge along in the
hopes that returning d * j somehow makes things okay. Happy, happy,
happy!

You'd be amazed.

What exactly does push_error() do?

It doesn't really matter, actually, for the purposes of the example. The
point is that the code, despite containing no goto statements, is still
a mess of spaghetti - the control flow is all over the floor.

 

[CBFalconer]

.... snip ...

Perhaps those who share my view about multiple returns are those who
have spent at least some of their career in the banking/insurance
world, and those who don't aren't? Just a thought.

Well, I have had sessions in banking, insurance, municipal taxes,
and payroll handling. But those are the exceptions, rather than
the rule. There may be something to your conjecture.

 

[CBFalconer]

.... snip ...

It doesn't really matter, actually, for the purposes of the example.
The point is that the code, despite containing no goto statements,
is still a mess of spaghetti - the control flow is all over the floor.

I can't believe you went to the trouble of creating that horror for
the article, so it must have existed somewhere. Condolences,
although that sort of thing is probably what keeps you in business.

 

[Army1987]

double ctx2174_frob_foo(foo *f, bar *b, baz *z, quux *q)
{
int i, j, k, l, m, n, o, p;
double foo_rate = 1.0;
double d = get_init_d_from_db(f->red, b->arney);

if(d < 0)
{
push_error(__FILE__, __LINE__, "ctx2174_frob_foo", "bad d");
return d;
}

for(i = b->a->barracus->lowentry;
i < q->u->ality->street;
i += z->e->bedee)
{
j = f->rank + b->ill->bob;
d += b->ubble * f->roth[j / 2] * q->uestion;
foo_rate *= d / b->arrack->room[j];
if(z->ygote > foo_rate)
{
for(k = 0; k / j > f->eed->the->world; k += i)
{
if(k + 4 < 3 / j)
{
push_error(__FILE__, __LINE__,
"ctx2174_frob_foo", "bad k/j ratio");
return d * j;
}
if(k * j > foo_rate / 3.14159)
{
f->lipside->folksy *= get_mod_x_from_db(z->ymurgy, j);
if(f->lipside->folksy / d < b->arrack->room / 9)
{
i++;
continue;
}
f->rank += 2;
if(f->rank < b->ill->b->ill)
{
z->o->o->keeper += d;
return foo_rate;
}
}
}
if(d * f->rank < b->alder->dash && f->orte < b->attledress)
{
return foo_rate / d;
}
}
for(l = 0, m = z->yx;
l < m;
l += b->ill->joe, m += b->ill->bob)
{
d += z->ygote * f->lipside->folksy[l];
if(get_mod_x_from_db(z->ymurgy, l) > m)
{
f->lipside->rocky[l] = get_mod_x_from_db(z->ymurgy, m);
return foo_rate * f->eed->the->fish;
}
else if(f->antasy->foot->ball < q->ual->ityof->life)
{
f->lipside->rap = get_mod_y_from_db(z->yzygy, m - l);
break;
}
else if(f->orei->gnle->gion < b->ill->jean)
{
f->lipside->country = get_mod_x_from_db(z->orch, l + --m);
}
foo_rate += f->ast->andl->oose[l];
do
{
n = 17 * q->uinine[m] *
get_magic_number((b->ilbo + f->rodo) / 2);
if(n < rune_caster(i, m))
{
return f->eed->the->tor->toise * 6;
}
} while(n > m);
}
}

for(i = ....

etc etc. Imagine another ten or twelve similar loops, each packed to the
gunwhales with other loops, each seemingly random in structure and yet
crammed with business logic (and if ever there were an oxymoron
contest, "business logic" would definitely be my entry), with returns -
some of them seemingly unrelated in nature to the others - scattered
liberally through the loops, and a final

return foo_rate + q->uive->ring->wrec[k];
}

LOL. ROTFL. It made me laugh to tears for 5 minutes or so. Did you write it
to show the point (if so, you really have *way* too much time on your
hands)? Did you copy actual existing code and just replace identifiers? (Oh
my God. It sounds absurd but I fear that's not really impossible.)

 

[Richard Heathfield]

Army1987 said:

"Richard Heathfield" <[email protected]> ha scritto nel messaggio

Imagine another ten or twelve similar loops, each packed to
the gunwhales with other loops, each seemingly random in structure
and yet crammed with business logic (and if ever there were an
oxymoron contest, "business logic" would definitely be my entry),
with returns - some of them seemingly unrelated in nature to the
others - scattered liberally through the loops, and a final

return foo_rate + q->uive->ring->wrec[k];
}

LOL. ROTFL. It made me laugh to tears for 5 minutes or so. Did you
write it to show the point (if so, you really have *way* too much time
on your hands)? Did you copy actual existing code and just replace
identifiers? (Oh my God. It sounds absurd but I fear that's not really
impossible.)

A shorter example, whilst it would have been much quicker to provide,
could not have conveyed the sheer control flow horror which faces
programmers in some code shops. Whilst the code was not in fact copied
and hacked, it could easily have been, were it not for NDAs and
confidentiality clauses and so on. If you were to substitute businessy
identifier names into that example, it would not have looked out of
place in quite a few of the systems I've worked on. And of course it's
impossible to persuade most project leads that it's worth taking the
time to rewrite it properly.

 

[Flash Gordon]

CBFalconer wrote, On 30/03/07 14:49:

Richard Heathfield wrote:
... snip ...

Well, I have had sessions in banking, insurance, municipal taxes,
and payroll handling. But those are the exceptions, rather than
the rule. There may be something to your conjecture.

You get the same mess other industries where money or government
regulations (or both) are involved.

I don't mind multiple returns when the function is small enough to
understand, and for some 5 line functions they make it a lot clearer
IMHO, but otherwise they can be a real pain, especially when mixed up in
other control structures.

 

[Martin]

Thanks for your response Chuck. And my thanks to all other respondents too.

Which incorporates several guidelines. One is single point of exit.

I have heard about this single point of exit guideline and I can't say I'm
totally convinced. My take on this is simple. If *all* you do in the code at
the point of each error is

ret_val = FAIL;

and no other processing occurs except

return ret_val;

which is possibly many lines away in the code, then I see no reason why that
can't be replaced with

return FAIL;

then any 'else' blocks can be removed and the code back-dented accordingly.

In my particular case the code I had to deal with I transformed to the form:

if (error_1) { report error_1 condition; return FAIL; }
if (error_2) { report error_2 condition; return FAIL; }
if (error_3) { report error_3 condition; return FAIL; }
if (error_4) { report error_4 condition; return FAIL; }

{ var declarations
processing with knowledge error conditions dealt with
}
return SUCCESS;

The reporting of each of the error conditions is handled differently in the
code I am modifying - a different function call for each one. The errors are
also detected in different ways. The question is, how would you re-write
this using the "single point of exit guideline". Not very elegantly, IMO.

the controlling condition is easily visible from the indentation

In this case, which controlling condition?

A third is that the short condition comes
first, easing finding the controlling condition via the
indentation.

Again, I don't think in this case it would apply.

 

[Ian Collins]

Thanks for your response Chuck. And my thanks to all other respondents too.



I have heard about this single point of exit guideline and I can't say I'm
totally convinced. My take on this is simple. If *all* you do in the code at
the point of each error is

ret_val = FAIL;

and no other processing occurs except

return ret_val;

which is possibly many lines away in the code, then I see no reason why that
can't be replaced with

return FAIL;

But what happens if the function might have to clean up before it returns?

If you can't see the entire function on on screen, it's too long.
Your sig is broken, the delimiter should be "-- "

 

[CBFalconer]

I can't believe you went to the trouble of creating that horror for
the article, so it must have existed somewhere. Condolences,
although that sort of thing is probably what keeps you in business.

Looking more closely it is actually much better structured than the
real life horrors I have seen, not to mention the identifiers. So
I guess you have too much time on your hands, and got caught up in
the structuring assistance of your editor.

 

[Richard Heathfield]

CBFalconer said:

Looking more closely it is actually much better structured than the
real life horrors I have seen, not to mention the identifiers.

Sorry about that. I was going for realism, but I didn't want to spend
*too* much time on it.

So
I guess you have too much time on your hands,

As I explained earlier, a shorter example would not have been adequate.
In any case, I'm a fast typist and it doubled as a coffee break.

and got caught up in
the structuring assistance of your editor.

Um, what structuring assistance? I use vim.