Error handling - How would you write it?

[Michael B Allen]

This is a highly opinionated question but I'd like to know what people
think in this matter.

When initializing / creating a group of objects together I usually
consolidate the error handling like the following:

if (foo_init(&f) == -1 ||
(b = bar_new()) == NULL ||
some_function(a, b, c) == -1) {
ERR(errno);
bar_del(b);
return -1;
}

With small conditionals this isn't too bad but sometimes it can get
pretty ugly. Would you rather see this expanded like:

if (foo_init(&f) == -1) {
ERR(errno);
return -1;
}
if ((b = bar_new()) == NULL) {
ERR(errno);
return -1;
}
if (some_function(a, b, c) == -1) {
ERR(errno);
bar_del(b);
return -1;
}

Or how would you write this and why?

Mike

Note: In this listing I use _new to indicate a function that allocates
resources that must be released as opposed to _init which does not. Also
ERR can be considered a macro that tracks error state for debugging
purposes.

 

[mm]

Hi,
those two examples (shrinked and expanded) are not the same - what if
foo_init() failes...
then you call bar_del() on "b" which is not initialized yet...
(ok, you can initialize it to 0 before and handle it in bar_del())

I would do a hybrid:

if( foo_init() || bar_new()) FAILED;

if( some_function()) FAILED; and bar_del()


marek

 

[Michael B Allen]

Hi,
those two examples (shrinked and expanded) are not the same - what if
foo_init() failes...
then you call bar_del() on "b" which is not initialized yet...

Well let's say b is initialized to NULL first and bar_del just returns
an error if b is NULL.

Mike

 

[Marek Przeczek]

well, then i would use your example - everything together in one if() since
it is simplier and shorter...
maybe because of transparency i would define that non-zero return value
means error and then you can write just:

if( foo_init() || (b = bar_init()) || sample_function()) {

failed;
message;
}

to remove unnecessary comparisons and brackets which only make code
unreadable...

marek

 

[Marek Przeczek]

sorry, there should be

!(b=bar_new())

marek

 

[CBFalconer]

*** top posting fixed ***

those two examples (shrinked and expanded) are not the same - what
if foo_init() failes...
then you call bar_del() on "b" which is not initialized yet...
(ok, you can initialize it to 0 before and handle it in bar_del())

I would do a hybrid:

if( foo_init() || bar_new()) FAILED;

if( some_function()) FAILED; and bar_del()

Please don't top-post.

You are right about the unitialized failure. However I would
operate as follows:

if (foo_init(&f) == -1) || (NULL == (b = bar_new())) {
ERR(errno); return -1;
}
else if some_function(a, b, c) == -1) {
ERR(errno); bar_del(b); return -1;
}
else {
/* all well - carry on */
}

another version could preinitialize b and use it as an error flag

b = NULL
if (-1 == foo_init(&f))
|| (NULL == (b = bar_new()))
|| (-1 == some_function(a, b, c)) {
if (b) bar_del(b);
ERR(errno); return -1;
}
else {
/* all well - carry on */
}

 

[websnarf]

This is a highly opinionated question but I'd like to know what people
think in this matter.

When initializing / creating a group of objects together I usually
consolidate the error handling like the following:

if (foo_init(&f) == -1 ||
(b = bar_new()) == NULL ||
some_function(a, b, c) == -1) {
ERR(errno);
bar_del(b);
return -1;
}

This code is very likely wrong. If foo_init() returns -1, that leaves
b uninitialized, and bar_del(b) will very likely be undefined. You
should swap the order of the foo_init(&f) and (b = bar_new())
computations.

With small conditionals this isn't too bad but sometimes it can get
pretty ugly. Would you rather see this expanded like:

if (foo_init(&f) == -1) {
ERR(errno);
return -1;
}
if ((b = bar_new()) == NULL) {
ERR(errno);
return -1;
}
if (some_function(a, b, c) == -1) {
ERR(errno);
bar_del(b);
return -1;
}

Or how would you write this and why?

Its a simple question of maintenance -- which is easier to maintain,
more lines of code, or fewer lines of code? As long as you can make
the shorter version clear and explicit about what its doing, it will
always be preferable.

As to this specific example, the only real issue is that you might be
interested in exactly *how* the function has failed. In which case you
prefer the second form, except that instead of returning -1, I would
suggest returning -__LINE__ (unless you have hidden a usage of __LINE__
in the ERR macro.)

 

[Netocrat]

This is a highly opinionated question but I'd like to know what people
think in this matter.

When initializing / creating a group of objects together I usually
consolidate the error handling like the following:

if (foo_init(&f) == -1 ||
(b = bar_new()) == NULL ||
some_function(a, b, c) == -1) {
ERR(errno);
bar_del(b);
return -1;
}

My preference is the way that you have done it - which is as concise as
possible. The only change I would make is to replace the comparison with
NULL with a ! test. Also if speed were very important then I may separate
out the bar_new test so that there wouldn't be any situation where bar_del
was called unnecessarily. But I would have to be convinced that wastage
was occurring before doing that.

Would you rather see this expanded like:

if (foo_init(&f) == -1) {
ERR(errno);
return -1;
}
if ((b = bar_new()) == NULL) {
ERR(errno);
return -1;
}
if (some_function(a, b, c) == -1) {
ERR(errno);
bar_del(b);
return -1;
}

Definitely not. Much less readable to me and it's repetitive and
space-wasting for no real gain. Why write the same code three times when
you can write it once?

 

[Kevin Bagust]

This is a highly opinionated question but I'd like to know what people
think in this matter.

When initializing / creating a group of objects together I usually
consolidate the error handling like the following:

if (foo_init(&f) == -1 ||
(b = bar_new()) == NULL ||
some_function(a, b, c) == -1) {
ERR(errno);
bar_del(b);
return -1;
}


Or how would you write this and why?

It would depend on how the three functions are related. If all three are
related or completely unrelated I would use the above, but in this case
it looks like bar_new and some_function are related as they both use b,
where as foo_init seems to be completely unrelated so I would split it
into two sections:

if ( foo_init(&f) == -1 ) {
ERR(errno);
return -1;
}

if (( b = bar_new()) == NULL ||
somefunction(a, b, c) == -1 ) {
ERR(errno);
bar_del(b);
return -1;
}

This helps to show what is related.

Kevin

 

[Steffen Buehler]

It would depend on how the three functions are related. If all three
are related or completely unrelated I would use the above,

I wouldn't use it in any case. This is a matter of taste, of course, but
the OP asked for it.

but in
this case it looks like bar_new and some_function are related as they
both use b, where as foo_init seems to be completely unrelated so I
would split it into two sections:

if ( foo_init(&f) == -1 ) {
ERR(errno);
return -1;
}

To my mind, a return in the middle of a code is always quite dangerous.
Always think of the fact that you have to maintain it some years later
or (even worse) someone else has to do it. You might well oversee the
small and shy "return" and the fact that the code below could be dead.
One entry and one exit. Like in RL.

if (( b = bar_new()) == NULL ||
somefunction(a, b, c) == -1 ) {
ERR(errno);
bar_del(b);
return -1;
}

What's so ugly separating these to ifs as well? The code becomes far
more readable and maintainable, I think. The times of 4K prog memories
are over. We can be a little more verbose. Yes - the or statement might
be more elegant, but again: the next time someone has to understand
100000 lines in this style it could take quite more time which is money
or at least leisure.

Best regards
Steffen

 

[Christopher Benson-Manica]

To my mind, a return in the middle of a code is always quite dangerous.
Always think of the fact that you have to maintain it some years later
or (even worse) someone else has to do it. You might well oversee the
small and shy "return" and the fact that the code below could be dead.

On the other hand, I don't think it's worth writing more convoluted
code merely to avoid having multiple return points in a function.
Take a simplistic search function:

int find_idx( int *array, int count, int value )
{
int idx, jdx=-1;
if( array ) {
for( idx=0; idx < count; idx++ ) {
if( array[idx] == value ) {
jdx=idx;
break;
}
}
}
return jdx;
}

versus

int find_idx( int *array, int count, int value )
{
int idx;
if( !array ) {
return -1;
}
for( idx=0; idx < count; idx++ ) {
if( array[idx] == value ) {
return idx;
}
}
return -1;
}

I believe the second approach is much clearer than the first approach.

 

[Default User]

On the other hand, I don't think it's worth writing more convoluted
code merely to avoid having multiple return points in a function.

Many company coding standards forbid multiple returns. It's something
that I try to adhere to in personal code as well, although not as
rigorously as when I'm writing it here (as at home there aren't any
peer reviews to worry about).

It certainly makes things easier for code testers and reviewers to have
just one return, always at the bottom of the function.

In the case of those arg check/init code, our standard method was
something like:

/* assume error status macros have been defined */
char error_message[80];
int status = SUCCESS;
int retval;

if ((retval = foo_init(&f)) == -1)
{
status = INIT_FAILED;
sprintf(error_message, "Init failed with error code: %d", retval);
}
else if ((b = bar_new()) == NULL)
{
status = ALLOC_FAILED;
strcpy(error_message, "bar_new failed to allocate memory");
}
else if ((retval = some_function(a, b, c)) == -1)
{
status = SOMETHING_FAILED;
strcpy(error_message, "some_function failed with error code: %d",
retval);
}

if (status == SUCCESS)
{
/* main processing section */
}

if (status != SUCCESS) /* there may have been errors in main sec */
{
report_error(status, error_message);
}

return status;



Brian

 

[CBFalconer]

To my mind, a return in the middle of a code is always quite
dangerous. Always think of the fact that you have to maintain it
some years later or (even worse) someone else has to do it. You
might well oversee the small and shy "return" and the fact that
the code below could be dead.

On the other hand, I don't think it's worth writing more
convoluted code merely to avoid having multiple return points in
a function. Take a simplistic search function:

int find_idx( int *array, int count, int value )
{
int idx, jdx=-1;
if( array ) {
for( idx=0; idx < count; idx++ ) {
if( array[idx] == value ) {
jdx=idx;
break;
}
}
}
return jdx;
}

versus

int find_idx( int *array, int count, int value )
{
int idx;
if( !array ) {
return -1;
}
for( idx=0; idx < count; idx++ ) {
if( array[idx] == value ) {
return idx;
}
}
return -1;
}

I believe the second approach is much clearer than the first
approach.

OTOH hand consider this mild rework of your first:

int find_idx( int *array, int count, int value )
{
int idx;

if (array)
for (idx = 0; idx < count; idx++)
if (array[idx] == value) return idx;
return -1;
}

or

int find_idx( int *array, int count, int value )
{
if (array)
while (count--)
if (array[count] == value) return count;
return -l;
}

which will find things in a different order. All are vulnerable to
a negative count input. This can be avoided by adding "&& (count >
0)" to the outer array test.

 

[Christopher Benson-Manica]

int find_idx( int *array, int count, int value )
{
int idx;

if (array)
for (idx = 0; idx < count; idx++)
if (array[idx] == value) return idx;
return -1;
}

int find_idx( int *array, int count, int value )
{
if (array)
while (count--)
if (array[count] == value) return count;
return -l;
}

which will find things in a different order. All are vulnerable to
a negative count input. This can be avoided by adding "&& (count >
0)" to the outer array test.

Only the second version is vulnerable to a negative count input; the
loop in the first example will merely be executed zero times if count
is negative. Both versions are admirably concise, although I'm not
sure they satisfy Steffen's desire for a single return point.

I would also suggest that iterating backward is not as intuitive (not
to say counterintuitive) as going forward, but that may just be me.