Only one point of return

Discussion in 'C++' started by cppquester@googlemail.com, Aug 1, 2007.

  1. Guest

    A colleague told me that there is a rule about good stype that a
    function in C++ should have only one point of return (ie. return
    statement). Otherwise there might be trouble.
    I never heard about it and doubt it.
    Anybody heard of it? What would be the advantage?

    Regards,
    Marc

    Example:

    bool f()
    {
    if( !pointer1) return false;
    pointer1->doSomething();

    if( !pointer2) return false;
    pointer2->doSomething1();

    return true;
    }

    vs.

    bool f()
    {
    bool retVal=true;
    if( pointer1)
    {
    pointer1->doSomething();
    }
    else
    retVal=false;

    if( pointer2)
    {
    pointer2->doSomething();
    }
    else
    retVal=false;

    return retVal;
    }
    , Aug 1, 2007
    #1
    1. Advertising

  2. Ian Collins Guest

    wrote:
    > A colleague told me that there is a rule about good stype that a
    > function in C++ should have only one point of return (ie. return
    > statement). Otherwise there might be trouble.
    > I never heard about it and doubt it.
    > Anybody heard of it? What would be the advantage?
    >

    It is a fairly common coding standard.

    In my opinion it makes more sense for C than C++, provided your code is
    exception safe an early return shouldn't do any harm. Some may argue
    that a single point of return makes debugging easier.

    A common problem early returns with either C or C++ that isn't exception
    safe is resource leaks where the resource in question is only released
    at the end of the function or before the last return.

    --
    Ian Collins.
    Ian Collins, Aug 1, 2007
    #2
    1. Advertising

  3. * :
    > A colleague told me that there is a rule about good stype that a
    > function in C++ should have only one point of return (ie. return
    > statement). Otherwise there might be trouble.
    > I never heard about it and doubt it.
    > Anybody heard of it? What would be the advantage?


    It's called SESE (Single Entry Single Exit), as opposed to SEME (Single
    Entry Multiple Exit).

    SESE is a good idea in languages like C, which have no provision for
    cleaning up automatically on function return. There, when maintaining
    code, you don't want cleanup code to be jumped over by an early return.

    C++ code, on the other hand, must be prepared to handle exceptions at
    any point, i.e. it's multiple exit at the outset. Or at least, if it's
    written by competent people, it will be designed for that, using RAII.
    So there's much less that can go wrong by using early normal returns,
    and the only factor should be what's more /clear/, in the context of a
    given function.



    > Regards,
    > Marc
    >
    > Example:
    >
    > bool f()
    > {
    > if( !pointer1) return false;
    > pointer1->doSomething();
    >
    > if( !pointer2) return false;
    > pointer2->doSomething1();
    >
    > return true;
    > }


    Bad, because it uses global variables, and is unclear.

    Are you really want it to do one of the things without doing both?

    But if so, try

    bool f()
    {
    if( pointer1 != 0 )
    {
    pointer1->doSomething();
    if( pointer1 != 0 )
    {
    pointer2->doSomething1();
    return true;
    }
    }
    return false;
    }


    > vs.
    >
    > bool f()
    > {
    > bool retVal=true;
    > if( pointer1)
    > {
    > pointer1->doSomething();
    > }
    > else
    > retVal=false;
    >
    > if( pointer2)
    > {
    > pointer2->doSomething();
    > }
    > else
    > retVal=false;
    >
    > return retVal;
    > }


    Urgh. :)

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
    Alf P. Steinbach, Aug 1, 2007
    #3
  4. duane hebert Guest

    <> wrote in message
    news:...
    >A colleague told me that there is a rule about good stype that a
    > function in C++ should have only one point of return (ie. return
    > statement). Otherwise there might be trouble.
    > I never heard about it and doubt it.
    > Anybody heard of it? What would be the advantage?
    >
    > Regards,
    > Marc
    >
    > Example:
    >
    > bool f()
    > {
    > if( !pointer1) return false;
    > pointer1->doSomething();
    >
    > if( !pointer2) return false;
    > pointer2->doSomething1();
    >
    > return true;
    > }
    >
    > vs.
    >
    > bool f()
    > {
    > bool retVal=true;
    > if( pointer1)
    > {
    > pointer1->doSomething();
    > }
    > else
    > retVal=false;
    >
    > if( pointer2)
    > {
    > pointer2->doSomething();
    > }
    > else
    > retVal=false;
    >
    > return retVal;
    > }


    This second example is different than the first since
    it goes to the pointer2 part even if pointer1 doesn't
    exist.
    Everyone has an opinion but IMO in C++, maintaining
    a single entry point leads to more obfuscated code
    rather than less (which is, I believe, the argument in
    favor of it.)
    duane hebert, Aug 1, 2007
    #4
  5. duane hebert Guest

    "duane hebert" <> wrote in message
    news:9p%ri.51852$...
    > This second example is different than the first since
    > it goes to the pointer2 part even if pointer1 doesn't
    > exist.
    > Everyone has an opinion but IMO in C++, maintaining
    > a single entry point leads to more obfuscated code


    a single exit point (duh)
    duane hebert, Aug 1, 2007
    #5
  6. werasm Guest

    Alf P. Steinbach wrote:
    but if so, try
    >
    > bool f()
    > {
    > if( pointer1 != 0 )
    > {
    > pointer1->doSomething();
    > if( pointer1 != 0 )
    > {
    > pointer2->doSomething1();
    > return true;
    > }
    > }
    > return false;
    > }



    Hmmm, or:

    bool f()
    {
    if( pointer1 == 0 ){ return false; }
    pointer1->doSomething();
    if( pointer2 == 0 ){ return false; }
    pointer2->doSomething();
    return true;
    }

    I like this because it seems less complex to read and understand, but
    the rationale is of course subjective (as is SESE and SEME in C++,
    depending to who you speak). I like getting the pre-conditions out of
    the way before handling the meat. SEME for me seems to do this better
    than SESE.

    Another alternative...

    template <class T>
    class ExistentPtr
    {
    public:
    ExistentPtr
    ( T*& p );
    T& operator*() const; //throws if pointer_ NULL.
    T* operator->() const; //throws if pointer_ NULL.

    private:
    T*& pointer_;
    };

    bool f()
    {
    ExistentPtr<Type> p1( pointer1 );
    ExistentPtr<Type> p2( pointer2 );

    try
    {
    p1->doSomething();
    p2->doSomething();
    return true;
    }
    catch( const null_ptr_error& )
    {
    return false;//
    }
    }

    Perhaps, if a function is written properly, it may never require
    visible ifs
    for the sake of handling errors, but this is very subjective.

    Regards,

    Werner
    werasm, Aug 1, 2007
    #6
  7. * werasm:
    > Alf P. Steinbach wrote:
    > but if so, try
    >> bool f()
    >> {
    >> if( pointer1 != 0 )
    >> {
    >> pointer1->doSomething();
    >> if( pointer1 != 0 )
    >> {
    >> pointer2->doSomething1();
    >> return true;
    >> }
    >> }
    >> return false;
    >> }

    >
    >
    > Hmmm, or:
    >
    > bool f()
    > {
    > if( pointer1 == 0 ){ return false; }
    > pointer1->doSomething();
    > if( pointer2 == 0 ){ return false; }
    > pointer2->doSomething();
    > return true;
    > }
    >
    > I like this because it seems less complex to read and understand, but
    > the rationale is of course subjective (as is SESE and SEME in C++,
    > depending to who you speak).


    Both versions are SEME. Yours is less clear because you can't tell at a
    glance under which conditions it will return true. You have to
    laboriously analyse the complete code in order to establish that.


    > I like getting the pre-conditions out of
    > the way before handling the meat.


    That's always necessary for precondition checking. Here we have no
    function level preconditions. But if we had, then checking them after
    they apply would not be a matter of like or dislike, it would simply be
    incorrect with possible undefined behavior.


    > SEME for me seems to do this better than SESE.


    Often yes. Both versions above are SEME.


    > Another alternative...
    >
    > template <class T>
    > class ExistentPtr
    > {
    > public:
    > ExistentPtr
    > ( T*& p );
    > T& operator*() const; //throws if pointer_ NULL.
    > T* operator->() const; //throws if pointer_ NULL.
    >
    > private:
    > T*& pointer_;
    > };
    >
    > bool f()
    > {
    > ExistentPtr<Type> p1( pointer1 );
    > ExistentPtr<Type> p2( pointer2 );
    >
    > try
    > {
    > p1->doSomething();
    > p2->doSomething();
    > return true;
    > }
    > catch( const null_ptr_error& )
    > {
    > return false;//
    > }
    > }


    Is both needlessly inefficient and unclear. Are the conditions under
    which p2->doSomething() is executed, intentional or an arbitrary side
    effect of using ExistenPtr? Impossible to tell, and that's a nightmare
    for the one maintaining the code, who must then check all /calling/ code
    for expectations about f's behavior (or lack of behavior).


    > Perhaps, if a function is written properly, it may never require
    > visible ifs
    > for the sake of handling errors, but this is very subjective.


    What makes you think the original example was handling any errors? It
    looked like normal case code to me. For errors, terminate or throw.

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
    Alf P. Steinbach, Aug 1, 2007
    #7
  8. werasm Guest

    Alf P. Steinbach wrote:


    > Both versions are SEME. Yours is less clear because you can't tell at a
    > glance under which conditions it will return true. You have to
    > laboriously analyse the complete code in order to establish that.


    Yes, true (both SEME), but I don't agree on the clarity. I prefer
    (that is probably subjective) completing the code that may cause the
    rest not to execute (as in the example), where in your case
    your if nesting becomes deeper (and for slightly longer functions
    very deep quite soon).

    > That's always necessary for precondition checking. Here we have no
    > function level preconditions.


    Well yes, perhaps they cannot be considered function level pre-
    conditions,
    but they certainly are pre-conditions to the code that follow, and
    prevent
    the function from continuing if they are not met.

    > Often yes. Both versions above are SEME.


    Yes, no arguing there, true.

    > > Another alternative...
    > >
    > > template <class T>
    > > class ExistentPtr
    > > {
    > > public:
    > > ExistentPtr
    > > ( T*& p );
    > > T& operator*() const; //throws if pointer_ NULL.
    > > T* operator->() const; //throws if pointer_ NULL.
    > >
    > > private:
    > > T*& pointer_;
    > > };
    > >
    > > bool f()
    > > {
    > > ExistentPtr<Type> p1( pointer1 );
    > > ExistentPtr<Type> p2( pointer2 );
    > >
    > > try
    > > {
    > > p1->doSomething();
    > > p2->doSomething();
    > > return true;
    > > }
    > > catch( const null_ptr_error& )
    > > {
    > > return false;//
    > > }
    > > }

    >
    > Is both needlessly inefficient and unclear. Are the conditions under
    > which p2->doSomething() is executed, intentional or an arbitrary side
    > effect of using ExistenPtr?


    In the example the condition under which p2->doSomething() is executed
    is always a function of p1->doSomething completing successfully and
    of p2's existence. The pointer wrapper should make the second
    pre-condition clear. Of course, p1->doSomething()'s successful
    completion is a function of p1's existence, which is naturally a
    pre-condition to p2.

    > Impossible to tell, and that's a nightmare
    > for the one maintaining the code, who must then check all /calling/ code
    > for expectations about f's behavior (or lack of behavior).


    Not if ExistentPtr's behavior is known to throw null_ptr_error if the
    pointer it wraps temporarily does not exist.

    > What makes you think the original example was handling any errors? It
    > looked like normal case code to me. For errors, terminate or throw.


    Yes, there is truth in this. The fact that the weak associations did
    not exist
    may imply that that part of functionality does not apply. I may have
    considered it erroneous prematurely. If it was handling an error, then
    code like
    this would have made sense to me:

    ExistentPtr<Type> p1( pointer1 );
    ExistentPtr<Type> p2( pointer2 );

    p1->doSomething();
    p2->doSomething();

    Regards,

    Werner
    werasm, Aug 1, 2007
    #8
  9. * werasm:
    > Alf P. Steinbach wrote:
    >
    >
    >> Both versions are SEME. Yours is less clear because you can't tell at a
    >> glance under which conditions it will return true. You have to
    >> laboriously analyse the complete code in order to establish that.

    >
    > Yes, true (both SEME), but I don't agree on the clarity. I prefer
    > (that is probably subjective) completing the code that may cause the
    > rest not to execute (as in the example), where in your case
    > your if nesting becomes deeper (and for slightly longer functions
    > very deep quite soon).


    That's a design error. Don't do it. Keep functions reasonably small.

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
    Alf P. Steinbach, Aug 1, 2007
    #9
  10. James Kanze Guest

    On Aug 1, 11:09 am, Ian Collins <> wrote:
    > wrote:
    > > A colleague told me that there is a rule about good stype that a
    > > function in C++ should have only one point of return (ie. return
    > > statement). Otherwise there might be trouble.
    > > I never heard about it and doubt it.
    > > Anybody heard of it? What would be the advantage?


    > It is a fairly common coding standard.


    > In my opinion it makes more sense for C than C++, provided your code is
    > exception safe an early return shouldn't do any harm. Some may argue
    > that a single point of return makes debugging easier.


    The real argument is that it makes understanding the code and
    reasoning about the correction of the code a lot easier, so you
    don't have to debug. It's more than just common; I'd say that
    it's present almost always where code review is taken seriously
    (which in turn means everywhere where the quality of code is
    taken seriously).

    About the only exceptions I've seen is clearing out
    pre-conditions at the top of the function, and in a few cases,
    if the function consists only of a single switch or a single
    list of chained if/else if/else, with all branches ending in a
    return. (The latter case is rarely allowed, probably because it
    is difficult to formulate precisely.)

    --
    James Kanze (GABI Software) email:
    Conseils en informatique orientée objet/
    Beratung in objektorientierter Datenverarbeitung
    9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34
    James Kanze, Aug 2, 2007
    #10
  11. James Kanze Guest

    On Aug 1, 11:11 am, "Alf P. Steinbach" <> wrote:
    > * :


    > > A colleague told me that there is a rule about good stype that a
    > > function in C++ should have only one point of return (ie. return
    > > statement). Otherwise there might be trouble.
    > > I never heard about it and doubt it.
    > > Anybody heard of it? What would be the advantage?


    > It's called SESE (Single Entry Single Exit), as opposed to SEME (Single
    > Entry Multiple Exit).


    > SESE is a good idea in languages like C, which have no provision for
    > cleaning up automatically on function return. There, when maintaining
    > code, you don't want cleanup code to be jumped over by an early return.


    The issue has more to do with understanding and reasoning about
    code than with the any cleanup code.

    > C++ code, on the other hand, must be prepared to handle exceptions at
    > any point, i.e. it's multiple exit at the outset.


    I've noticed that in well written C++, any exceptions tend to
    cluster at the top of the function, in a manner coherent with
    the one frequently accepted exception: checking pre-conditions.
    You don't want to exit code at just any old point.

    > Or at least, if it's written by competent people, it will be
    > designed for that, using RAII. So there's much less that can
    > go wrong by using early normal returns, and the only factor
    > should be what's more /clear/, in the context of a given
    > function.


    > > Example:


    > > bool f()
    > > {
    > > if( !pointer1) return false;
    > > pointer1->doSomething();


    > > if( !pointer2) return false;
    > > pointer2->doSomething1();


    > > return true;
    > > }


    > Bad, because it uses global variables, and is unclear.


    > Are you really want it to do one of the things without doing both?


    > But if so, try


    > bool f()
    > {
    > if( pointer1 != 0 )
    > {
    > pointer1->doSomething();
    > if( pointer1 != 0 )
    > {
    > pointer2->doSomething1();
    > return true;
    > }


    Just curious, but what invariants hold here?

    > }
    > return false;
    > }


    Which is easily (and more clearly) rewritten as:

    bool
    f()
    {
    bool succeeded = false ;
    if ( pointer1 != NULL ) {
    pointer1->doSomething() ;
    if ( pointer2 != NULL ) {
    pointer2->doSomething() ;
    succeeded = true ;
    }
    }
    return succeeded ;
    }

    By naming the condition, you've adding important semantic
    content for the reader. Even better would be something like:

    bool
    maybe1()
    {
    if ( pointer1 != NULL ) {
    pointer1->doSomething() ;
    }
    return pointer1 != NULL ;
    }

    bool
    maybe2()
    {
    // Alternate way of writing it...
    // (only for those who swear by functional
    // programming:)
    return pointer2 != NULL
    && (pointer2->doSomething(), true) ;
    }

    bool
    f()
    {
    return maybe1() && maybe2() ;
    }

    (But like you, I have a great deal of difficulty imagining a
    case where you'd want to do half the job, and then report that
    you'd not done any of it.)

    --
    James Kanze (GABI Software) email:
    Conseils en informatique orientée objet/
    Beratung in objektorientierter Datenverarbeitung
    9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34
    James Kanze, Aug 2, 2007
    #11
  12. * James Kanze:
    > On Aug 1, 11:11 am, "Alf P. Steinbach" <> wrote:
    >> * :

    >
    >>> A colleague told me that there is a rule about good stype that a
    >>> function in C++ should have only one point of return (ie. return
    >>> statement). Otherwise there might be trouble.
    >>> I never heard about it and doubt it.
    >>> Anybody heard of it? What would be the advantage?

    >
    >> It's called SESE (Single Entry Single Exit), as opposed to SEME (Single
    >> Entry Multiple Exit).

    >
    >> SESE is a good idea in languages like C, which have no provision for
    >> cleaning up automatically on function return. There, when maintaining
    >> code, you don't want cleanup code to be jumped over by an early return.

    >
    > The issue has more to do with understanding and reasoning about
    > code than with the any cleanup code.


    Well, that's a generalization: cleanup code is just the most important
    in practice, because maintainance is more important than original
    development. You may be one of the lucky people who has not had to help
    maintain C systems with 600 to 1000 line functions, evolved haphazardly.
    I can assure that you such systems are very common, for you see, most
    programmers are average. And the average programmer who maintains such
    code has a tendency to introduce early returns that skip cleanup code
    (often this is later corrected by duplicating the cleanup code in
    question, which then gets out of synch, and so on and on).


    >> C++ code, on the other hand, must be prepared to handle exceptions at
    >> any point, i.e. it's multiple exit at the outset.

    >
    > I've noticed that in well written C++, any exceptions tend to
    > cluster at the top of the function, in a manner coherent with
    > the one frequently accepted exception: checking pre-conditions.
    > You don't want to exit code at just any old point.


    You're IMO right that it's best to not hide exits deeply nested in
    masses of other code, but C++ code must be /prepared/ to exit any point.


    >> Or at least, if it's written by competent people, it will be
    >> designed for that, using RAII. So there's much less that can
    >> go wrong by using early normal returns, and the only factor
    >> should be what's more /clear/, in the context of a given
    >> function.

    >
    >>> Example:

    >
    >>> bool f()
    >>> {
    >>> if( !pointer1) return false;
    >>> pointer1->doSomething();

    >
    >>> if( !pointer2) return false;
    >>> pointer2->doSomething1();

    >
    >>> return true;
    >>> }

    >
    >> Bad, because it uses global variables, and is unclear.

    >
    >> Are you really want it to do one of the things without doing both?

    >
    >> But if so, try

    >
    >> bool f()
    >> {
    >> if( pointer1 != 0 )
    >> {
    >> pointer1->doSomething();
    >> if( pointer1 != 0 )
    >> {
    >> pointer2->doSomething1();
    >> return true;
    >> }

    >
    > Just curious, but what invariants hold here?
    >
    >> }
    >> return false;
    >> }


    "Invariants" is not really meaningful here. But at the point you
    indicate, you can assert(pointer1!=0), so that's an invariant of sorts.
    Also, at that point pointer1-doSometing() has been executed.



    > Which is easily (and more clearly) rewritten as:
    >
    > bool
    > f()
    > {
    > bool succeeded = false ;
    > if ( pointer1 != NULL ) {
    > pointer1->doSomething() ;
    > if ( pointer2 != NULL ) {
    > pointer2->doSomething() ;
    > succeeded = true ;
    > }
    > }
    > return succeeded ;
    > }


    I wouldn't call that clear, rather completely /misleading/: judging from
    the original code, this function succeeds even when it does nothing.
    But using a result variable has its charms. E.g. you can set a
    conditional breakpoint on the single return statement. The cost is, as
    evidenced by the code above, less clarity. ;-)


    > By naming the condition, you've adding important semantic
    > content for the reader. Even better would be something like:
    >
    > bool
    > maybe1()
    > {
    > if ( pointer1 != NULL ) {
    > pointer1->doSomething() ;
    > }
    > return pointer1 != NULL ;
    > }
    >
    > bool
    > maybe2()
    > {
    > // Alternate way of writing it...
    > // (only for those who swear by functional
    > // programming:)
    > return pointer2 != NULL
    > && (pointer2->doSomething(), true) ;
    > }
    >
    > bool
    > f()
    > {
    > return maybe1() && maybe2() ;
    > }
    >
    > (But like you, I have a great deal of difficulty imagining a
    > case where you'd want to do half the job, and then report that
    > you'd not done any of it.)
    >



    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
    Alf P. Steinbach, Aug 2, 2007
    #12
  13. Jorgen Grahn Guest

    On Wed, 1 Aug 2007 08:55:31 -0400, duane hebert <> wrote:
    >
    > <> wrote in message
    > news:...
    >>A colleague told me that there is a rule about good stype that a
    >> function in C++ should have only one point of return (ie. return
    >> statement). Otherwise there might be trouble.

    ....
    > Everyone has an opinion but IMO in C++, maintaining
    > a single entry [he means exit] point leads to more obfuscated code
    > rather than less (which is, I believe, the argument in
    > favor of it.)


    My experience, too, kind of.

    I haven't been in a project where we had to follow the rule, but I
    have worked with people who have adopted it, to some extent.

    I can well believe that otherwise well-written code (where functions
    are the right size and have the right amount of responsibility) are
    often better and more elegant if they are rewritten with a single exit
    point.

    But if the code, the programmers and the time schedule are only
    half-decent and you enforce the single-exit-point rule (it's easy to
    spot and comment on in a code review) I believe you tend to end up
    with the ugliness and bugs that come with:

    - deeply nested if/else logic
    - numerous local boolean flags controlling the flow
    - the return value being set in half a dozen places, on different code
    paths

    For a concrete example: I hate reading a two-page function which ends
    with "return result;" and searching back for all places where 'result'
    is (or isn't) initialized with a relevant value.

    /Jorgen

    --
    // Jorgen Grahn <grahn@ Ph'nglui mglw'nafh Cthulhu
    \X/ snipabacken.dyndns.org> R'lyeh wgah'nagl fhtagn!
    Jorgen Grahn, Aug 3, 2007
    #13
  14. James Kanze Guest

    On Aug 2, 12:18 pm, "Alf P. Steinbach" <> wrote:
    > * James Kanze:
    > > On Aug 1, 11:11 am, "Alf P. Steinbach" <> wrote:
    > >> * :


    > >>> A colleague told me that there is a rule about good stype that a
    > >>> function in C++ should have only one point of return (ie. return
    > >>> statement). Otherwise there might be trouble.
    > >>> I never heard about it and doubt it.
    > >>> Anybody heard of it? What would be the advantage?


    > >> It's called SESE (Single Entry Single Exit), as opposed to SEME (Single
    > >> Entry Multiple Exit).


    > >> SESE is a good idea in languages like C, which have no provision for
    > >> cleaning up automatically on function return. There, when maintaining
    > >> code, you don't want cleanup code to be jumped over by an early return.


    > > The issue has more to do with understanding and reasoning about
    > > code than with the any cleanup code.


    > Well, that's a generalization:


    It's a relativisation. The original arguments in favor of SESE
    (e.g. by Dijkstra et al.) did not involve cleanup code. That
    doesn't mean that cleanup code isn't important in practice.
    Just that it wasn't the original motivation (and that even when
    cleanup code isn't an issue, e.g. when you are using RAII, the
    original arguments hold).

    > cleanup code is just the most important
    > in practice, because maintainance is more important than original
    > development. You may be one of the lucky people who has not had to help
    > maintain C systems with 600 to 1000 line functions, evolved haphazardly.


    I have. In such cases, there is only one solution: rewrite the
    code with smaller functions.

    > I can assure that you such systems are very common, for you see, most
    > programmers are average. And the average programmer who maintains such
    > code has a tendency to introduce early returns that skip cleanup code
    > (often this is later corrected by duplicating the cleanup code in
    > question, which then gets out of synch, and so on and on).


    The problem isn't just the programmers. "Average" programmers
    can write very good code, if they are managed correctly. Good
    code review, and coding guidelines, for example.

    > >> C++ code, on the other hand, must be prepared to handle exceptions at
    > >> any point, i.e. it's multiple exit at the outset.


    > > I've noticed that in well written C++, any exceptions tend to
    > > cluster at the top of the function, in a manner coherent with
    > > the one frequently accepted exception: checking pre-conditions.
    > > You don't want to exit code at just any old point.


    > You're IMO right that it's best to not hide exits deeply nested in
    > masses of other code, but C++ code must be /prepared/ to exit any point.


    It depends. I tend to prefer a transactional style, which
    verifies first that nothing can fail, before modifying any
    actual data. (But of course, it isn't always possible.) In
    practice, no throw guarantees are important for many idioms.

    [...]
    > >> Are you really want it to do one of the things without doing both?


    > >> But if so, try


    > >> bool f()
    > >> {
    > >> if( pointer1 != 0 )
    > >> {
    > >> pointer1->doSomething();
    > >> if( pointer1 != 0 )
    > >> {
    > >> pointer2->doSomething1();
    > >> return true;
    > >> }


    > > Just curious, but what invariants hold here?


    > >> }
    > >> return false;
    > >> }


    > "Invariants" is not really meaningful here.


    Invariants are always important.

    The problem here, of course, is that we have an abstract
    function, without any idea as to what it really means. So we
    don't know what invariants are relevant. The important point is
    that the structure of the code (indenting, etc.) very strongly
    suggests that the results of pointer2->doSomething1() may in
    fact hold, where as it is actually guaranteed that they don't.
    The indenting is a lie.

    > But at the point you
    > indicate, you can assert(pointer1!=0), so that's an invariant of sorts.
    > Also, at that point pointer1-doSometing() has been executed.


    It's the first I was thinking of. The indentation and program
    structure says one thing. (We're after the if, so the condition
    of the if is no longer guaranteed.) The actual situation is
    another. The indentation is lying.

    Of course, the original author, when writing the code, is very
    much aware of the preceding return, and the additional
    guarantees it introduces. So he writes code which takes
    advantage of them. The later maintenance programmer then has to
    try to figure out how it can work, when the invariant on which
    it depends apparently isn't guaranteed.

    > > Which is easily (and more clearly) rewritten as:


    > > bool
    > > f()
    > > {
    > > bool succeeded = false ;
    > > if ( pointer1 != NULL ) {
    > > pointer1->doSomething() ;
    > > if ( pointer2 != NULL ) {
    > > pointer2->doSomething() ;
    > > succeeded = true ;
    > > }
    > > }
    > > return succeeded ;
    > > }


    > I wouldn't call that clear, rather completely /misleading/: judging from
    > the original code, this function succeeds even when it does nothing.


    Who knows? I just guessed at a semantic signification for the
    bool. The whole function is, as you originally pointed out,
    problematic. It's very hard to discuss what is reasonable in a
    solution without knowing what the problem is.

    > But using a result variable has its charms.


    Yes. It allows naming the condition. This is very important
    when dealing with bool, since the name is all you really have to
    go on. (It's a totally different issue, but in my experience,
    it is very, very rare that a function should return bool. But
    there are obvious exceptions: just about any function whose name
    begins with "is", for example, or something like "contains(key)"
    on a map.)

    It allows allows the visual structure to correspond to the
    actual control flow; code which is outside of a block controled
    by an if doesn't depend on that if.

    --
    James Kanze (GABI Software) email:
    Conseils en informatique orientée objet/
    Beratung in objektorientierter Datenverarbeitung
    9 place Sémard, 78210 St.-Cyr-l'École, France, +33 (0)1 30 23 00 34
    James Kanze, Aug 3, 2007
    #14
  15. * James Kanze:
    > On Aug 2, 12:18 pm, "Alf P. Steinbach" <> wrote:
    >
    >>>> bool f()
    >>>> {
    >>>> if( pointer1 != 0 )
    >>>> {
    >>>> pointer1->doSomething();
    >>>> if( pointer1 != 0 )
    >>>> {
    >>>> pointer2->doSomething1();
    >>>> return true;
    >>>> }

    >
    >>> Just curious, but what invariants hold here?

    >
    >>>> }
    >>>> return false;
    >>>> }

    >
    >> "Invariants" is not really meaningful here.

    >
    > Invariants are always important.
    >
    > The problem here, of course, is that we have an abstract
    > function, without any idea as to what it really means. So we
    > don't know what invariants are relevant. The important point is
    > that the structure of the code (indenting, etc.) very strongly
    > suggests that the results of pointer2->doSomething1() may in
    > fact hold, where as it is actually guaranteed that they don't.
    > The indenting is a lie.


    An invariant about a particular place in the possible execution, holds
    about that place, and no other place.

    If the execution ever passes your comment, then at that point the
    invariant holds: pointer1 != 0, and pointer1->doSomething has been
    executed successfully (assuming that it throws on failure).

    This is trivial, and the indentation is correct, not a lie or misleading.

    There is in general no guarantee in C++ that execution will ever reach
    any particular point.

    Yet of course that does not invalidate the concept of invariants that
    hold at particular places that potentially can be reached (and will be
    reaced in normal code execution).

    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
    Alf P. Steinbach, Aug 3, 2007
    #15
  16. werasm Guest

    Alf P. Steinbach wrote:


    > That's a design error. Don't do it. Keep functions reasonably small.


    In all honesty, I'm an advocate of short functions - "One function
    one responsibility" as well. Originally I considered a similar
    solution to Kanze where two seperate functions existed, each
    returning boolean, but this function seemed to short to bother.
    Boolean also seems the wrong return value as this function has
    more than one reason for failure.

    I don't have that big a problem with what you've done really,
    in the context is was done. This is an unusual function as one
    does not know whether pointer2 becomes valid due to action on
    pointer1. Perhaps a better solution would have been to verify both
    pointers prior to performing the operations on them, but one does
    not know in this case whether its an error or intentional. IMO if
    intentional, this should have actually been two functions as
    performing these actions in one function allows state to be modified
    partially, so to speak (the function does not have commit/rollback
    semantics (fail or succeed)).

    Stating now that I always get pre-conditions out of the way early,
    would not be true. I just did not like your nesting, and in
    retrospect none of the proposed solutions - my response on
    yours included were sufficient. Of all I liked "return( doOne()
    && doTwo() );" the best, but a boolean return value is deceiving
    in this case. This should have been two functions.

    BTW. I have done maintenance for 3 years on a large, ugly 'oll
    C program. I know what it feels like to work through 300 liners (with
    which I don't agree - of course). If the responsibility of an
    abstraction
    is known, it does not make maintenance more difficult, on the
    contrary.

    Kind regards,

    Werner
    werasm, Aug 3, 2007
    #16
  17. * werasm:
    > Alf P. Steinbach wrote:
    >
    >
    >> That's a design error. Don't do it. Keep functions reasonably small.

    >
    > In all honesty, I'm an advocate of short functions - "One function
    > one responsibility" as well. Originally I considered a similar
    > solution to Kanze where two seperate functions existed, each
    > returning boolean, but this function seemed to short to bother.
    > Boolean also seems the wrong return value as this function has
    > more than one reason for failure.
    >
    > I don't have that big a problem with what you've done really,
    > in the context is was done. This is an unusual function as one
    > does not know whether pointer2 becomes valid due to action on
    > pointer1. Perhaps a better solution would have been to verify both
    > pointers prior to performing the operations on them, but one does
    > not know in this case whether its an error or intentional. IMO if
    > intentional, this should have actually been two functions as
    > performing these actions in one function allows state to be modified
    > partially, so to speak (the function does not have commit/rollback
    > semantics (fail or succeed)).
    >
    > Stating now that I always get pre-conditions out of the way early,
    > would not be true. I just did not like your nesting, and in
    > retrospect none of the proposed solutions - my response on
    > yours included were sufficient. Of all I liked "return( doOne()
    > && doTwo() );" the best, but a boolean return value is deceiving
    > in this case. This should have been two functions.
    >
    > BTW. I have done maintenance for 3 years on a large, ugly 'oll
    > C program. I know what it feels like to work through 300 liners (with
    > which I don't agree - of course). If the responsibility of an
    > abstraction
    > is known, it does not make maintenance more difficult, on the
    > contrary.


    Well I think we're into matters of personal preference.

    But about the translate-bools-to-exceptions-and-back-to-bools code: as
    part of being more complicated in the sense of figuring intended
    functionality (versus unintended), it swallows null_ptr_error exceptions
    from the two functions called, replacing with bool return. Depending on
    the specification of what the bool return means (i.e. does it indicate
    error or how much successful work as specified by arguments) that may be
    correct, or not. Most likely the OP didn't mean the function to swallow
    any exceptions, and in that case it's incorrect; anyway, not
    functionally the exact same.

    So I think of it as over-abstraction (also a little inefficient!); if we
    want always non-null arguments, that's better expressed as references.

    Cheers,

    - Alf


    --
    A: Because it messes up the order in which people normally read text.
    Q: Why is it such a bad thing?
    A: Top-posting.
    Q: What is the most annoying thing on usenet and in e-mail?
    Alf P. Steinbach, Aug 3, 2007
    #17
  18. werasm Guest

    Alf P. Steinbach wrote:

    > But about the translate-bools-to-exceptions-and-back-to-bools code: as
    > part of being more complicated in the sense of figuring intended
    > functionality (versus unintended), it swallows null_ptr_error exceptions
    > from the two functions called, replacing with bool return.


    Agreed (in retrospect). The intention of that pointer wrapper is
    mostly to
    initialize pointers that might be used as weak associations (I
    would not want to have a lecture on shared_ptrs or scoped_ptr
    as alternative). In this context it could have been used to
    determine the nullness, but not necessary. I made the initial
    assumption that the pointers should never be null (that it
    is an error), but as you rightly stated, then we should
    terminate or throw (if it was an error).

    A useful place for ExistentPtr is as wrapper for a
    raw pointer member - guaranteeing initialisation to
    NULL. Then throwing if the pointer is used without
    pointing to something. This prevents the sloppy pro-
    grammer (they do exist, unfortunately) from not
    initialising a pointer to NULL in the initialisation list.
    The problem with scoped_ptr is that the behaviour
    is undefined if it is not initialised. In practise though,
    it probably does the same thing.

    > So I think of it as over-abstraction (also a little inefficient!); if we
    > want always non-null arguments, that's better expressed as references.


    Yes, although a reference requires initialization at construction. We
    don't
    know whether in this case, references were plausible and that the re-
    lationships were actually known at the time of construction. IME I
    have
    found that good work- arounds exist most of the time when
    relationships
    aren't known at the time of construction. References aren't one of
    them.

    Regards,

    Werner
    werasm, Aug 3, 2007
    #18
  19. Kai-Uwe Bux Guest

    wrote:

    > A colleague told me that there is a rule about good stype that a
    > function in C++ should have only one point of return (ie. return
    > statement). Otherwise there might be trouble.
    > I never heard about it and doubt it.
    > Anybody heard of it?


    Sure.

    > What would be the advantage?


    Don't know. Some people think it makes it easier to argue correctness. I am
    not so sure about that, though. However, I can see the point: return
    statements are like goto statements; they jump to the end of the current
    routine. As with goto statements, you need to be careful.


    [snip]
    > bool f()
    > {
    > if( !pointer1) return false;
    > pointer1->doSomething();
    >
    > if( !pointer2) return false;
    > pointer2->doSomething1();
    >
    > return true;
    > }
    >
    > vs.
    >
    > bool f()
    > {
    > bool retVal=true;
    > if( pointer1)
    > {
    > pointer1->doSomething();
    > }
    > else
    > retVal=false;
    >
    > if( pointer2)
    > {
    > pointer2->doSomething();
    > }
    > else
    > retVal=false;
    >
    > return retVal;
    > }


    I am not really certain about that example. I tend to have multiple exits in
    situations like this:

    /*
    triple is like pair, except that it has fields "first",
    "second", and "third".
    */
    template < typename A, typename B, typename C >
    bool operator< ( triple< A, B, C > const & lhs,
    triple< A, B, C > const & rhs ) {
    // try deciding using first:
    if ( lhs.first < rhs.first ) {
    return ( true );
    }
    if ( rhs.first < lhs.first ) {
    return ( false );
    }
    // first was tied. try second:
    if ( lhs.second < rhs.second ) {
    return ( true );
    }
    if ( rhs.second < lhs.second ) {
    return ( false );
    }
    // first and second tied. try thrid:
    if ( lhs.third < rhs.third ) {
    return ( true );
    }
    if ( rhs.third < lhs.third ) {
    return ( false );
    }
    // all tied:
    return ( false );
    }

    or

    template < typename ForwardIter, typename Pred >
    bool forall ( ForwardIter from, ForwardIter to, Pred p ) {
    // returns true iff p is not false for all *i in the range [from,to)
    while ( from != to ) {
    if ( ! p(*from) ) {
    return ( false );
    }
    ++ from;
    }
    return ( true );
    }



    Best

    Kai-Uwe Bux
    Kai-Uwe Bux, Aug 3, 2007
    #19
  20. Jim Langston Guest

    <> wrote in message
    news:...
    >A colleague told me that there is a rule about good stype that a
    > function in C++ should have only one point of return (ie. return
    > statement). Otherwise there might be trouble.
    > I never heard about it and doubt it.
    > Anybody heard of it? What would be the advantage?
    >
    > Regards,
    > Marc
    >
    > Example:
    >
    > bool f()
    > {
    > if( !pointer1) return false;
    > pointer1->doSomething();
    >
    > if( !pointer2) return false;
    > pointer2->doSomething1();
    >
    > return true;
    > }
    >
    > vs.
    >
    > bool f()
    > {
    > bool retVal=true;
    > if( pointer1)
    > {
    > pointer1->doSomething();
    > }
    > else
    > retVal=false;
    >
    > if( pointer2)
    > {
    > pointer2->doSomething();
    > }
    > else
    > retVal=false;
    >
    > return retVal;
    > }


    This was one of the fundamentals of "top down design" that came out of
    trying to prevent spagetti code. The reasoning being it is much easier to
    understand which code is executed and which isn't without having to look
    farther up the function. It is a good idea to get used to, but I find it
    better to return earlier on exceptions when it's rather obvious.

    With short functions, however, it should not be a problem. I find that if
    I'm checking on the validity of something (returning bool) that it is easier
    to read with many return statements. Consider, which is easier to
    understand at first glance?

    bool CheckValue( Foo bar )
    {
    if ( bar.value )
    return false;
    if ( bar.otherval > 0 )
    return false;
    if ( bar.value == bar.othervalue )
    return false;
    return true;
    }

    bool CheckValue( Foo bar )
    {
    bool ReturnVal = true;
    if ( bar.value )
    ReturnVal = false;
    if ( bar.othervalue )
    ReturnVal = false;
    if ( bar.value == bar.othervalue )
    ReturnVal = false;
    return ReturnVal;
    }
    }
    Jim Langston, Aug 4, 2007
    #20
    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. Greenhorn
    Replies:
    15
    Views:
    807
    Keith Thompson
    Mar 6, 2005
  2. Replies:
    5
    Views:
    549
    Thomas J. Gritzan
    Oct 6, 2006
  3. Lakshmi Sreekanth

    If number is 0 return zero else return one

    Lakshmi Sreekanth, Sep 14, 2009, in forum: C Programming
    Replies:
    11
    Views:
    620
    Phil Carmody
    Sep 16, 2009
  4. yo
    Replies:
    1
    Views:
    390
    Andreas Perstinger
    Oct 13, 2011
  5. Saraswati lakki
    Replies:
    0
    Views:
    1,317
    Saraswati lakki
    Jan 6, 2012
Loading...

Share This Page