Error handling - How would you write it?

Discussion in 'C Programming' started by Michael B Allen, Jul 9, 2005.

  1. 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.
     
    Michael B Allen, Jul 9, 2005
    #1
    1. Advertising

  2. Michael B Allen

    mm Guest

    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" <> wrote in message
    news:p...
    > 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, Jul 9, 2005
    #2
    1. Advertising

  3. On Sat, 09 Jul 2005 20:09:10 +0200, mm wrote:

    > 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
     
    Michael B Allen, Jul 9, 2005
    #3
  4. 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


    "Michael B Allen" <> wrote in message
    news:p...
    > On Sat, 09 Jul 2005 20:09:10 +0200, mm wrote:
    >
    > > 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, Jul 9, 2005
    #4
  5. sorry, there should be

    !(b=bar_new())

    :) marek

    "Marek Przeczek" <> wrote in message
    news:dapabt$imv$...
    > 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
    >
    >
    > "Michael B Allen" <> wrote in message
    > news:p...
    > > On Sat, 09 Jul 2005 20:09:10 +0200, mm wrote:
    > >
    > > > 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, Jul 9, 2005
    #5
  6. Michael B Allen

    CBFalconer Guest

    *** top posting fixed ***
    mm wrote:
    > "Michael B Allen" <> wrote in message
    >
    >> 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;
    >> }

    .... snip ...
    >>
    >> 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.

    >
    > 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 */
    }

    --
    "If you want to post a followup via groups.google.com, don't use
    the broken "Reply" link at the bottom of the article. Click on
    "show options" at the top of the article, then click on the
    "Reply" at the bottom of the article headers." - Keith Thompson
     
    CBFalconer, Jul 9, 2005
    #6
  7. Michael B Allen

    Guest

    Michael B Allen wrote:
    > 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.)

    --
    Paul Hsieh
    http://www.pobox.com/~qed/
    http://bstring.sf.net/
     
    , Jul 10, 2005
    #7
  8. Michael B Allen

    Netocrat Guest

    On Sat, 09 Jul 2005 13:59:23 -0400, Michael B Allen wrote:

    > 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?
     
    Netocrat, Jul 10, 2005
    #8
  9. Michael B Allen

    Kevin Bagust Guest

    Michael B Allen wrote:
    > 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
     
    Kevin Bagust, Jul 10, 2005
    #9
  10. Kevin Bagust wrote:

    > 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
     
    Steffen Buehler, Jul 11, 2005
    #10
  11. Steffen Buehler <> wrote:

    > 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.

    --
    Christopher Benson-Manica | I *should* know what I'm talking about - if I
    ataru(at)cyberspace.org | don't, I need to know. Flames welcome.
     
    Christopher Benson-Manica, Jul 11, 2005
    #11
  12. Michael B Allen

    Default User Guest

    Christopher Benson-Manica wrote:
    > Steffen Buehler <> wrote:
    >
    > > 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.



    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
     
    Default User, Jul 11, 2005
    #12
  13. Michael B Allen

    CBFalconer Guest

    Christopher Benson-Manica wrote:
    > Steffen Buehler <> wrote:
    >
    >> 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.

    --
    "If you want to post a followup via groups.google.com, don't use
    the broken "Reply" link at the bottom of the article. Click on
    "show options" at the top of the article, then click on the
    "Reply" at the bottom of the article headers." - Keith Thompson
     
    CBFalconer, Jul 11, 2005
    #13
  14. CBFalconer <> wrote:

    > 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.

    --
    Christopher Benson-Manica | I *should* know what I'm talking about - if I
    ataru(at)cyberspace.org | don't, I need to know. Flames welcome.
     
    Christopher Benson-Manica, Jul 11, 2005
    #14
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. =?Utf-8?B?SklNLkgu?=

    Q: how would you write this

    =?Utf-8?B?SklNLkgu?=, Feb 17, 2005, in forum: ASP .Net
    Replies:
    2
    Views:
    309
    William F. Robertson, Jr.
    Feb 17, 2005
  2. Jeff Rush
    Replies:
    4
    Views:
    303
    Michele Simionato
    Apr 26, 2007
  3. aleksa

    How would you write this?

    aleksa, Apr 24, 2010, in forum: C Programming
    Replies:
    13
    Views:
    435
    Tim Rentsch
    Apr 29, 2010
  4. Replies:
    2
    Views:
    100
    matt neuburg
    Oct 12, 2006
  5. Frank Meyer
    Replies:
    12
    Views:
    174
    Gregory Brown
    Aug 12, 2007
Loading...

Share This Page