TITLE: Guru#2 - author's answer

(Newsgroups: comp.lang.c++.moderated, 3 Mar 97)

[ Just for completeness, I included the author's "answer" to the
  Guru#2 question. -adc ]


SUTTER: herbs@cntc.com (Herb Sutter)

Congratulations to Richard Sullivan, the guru of the week!  Richard was the
first to supply a reasonably correct answer addressing all major parts of
last week's problem.

Solution follows:

>    string FindAddr( list<Employee> l, string name )
>    {
>        for( list<Employee>::iterator i = l.begin(); i != l.end(); i++ )
>        {
>            if( *i == name )
>            {
>                return (*i).addr;
>            }
>        }
>        return "";
>    }

Believe it or not, these five lines have three obvious cases of unnecessary
temporaries, two subtler ones, and a red herring.


1 & 2. string FindAddr( list<Employee> l, string name )
                        ^^^^^^^1^^^^^^^^  ^^^^^2^^^^^

    The parameters should be const references.  Pass-by-value copies both
    the list and the string, which can be expensive.

    [Rule]      Prefer passing const& instead of copied values.


3. for( /*...*/ i++ )
                ^3^

    This one was more subtle.  Preincrement is more efficient than
    postincrement, because for postincrement the object must increment
    itself and then return a temporary containing its old value.  Note that
    this is true even for builtins like int!

    [Guideline] Prefer preincrement, avoid postincrement.


4. if( *i == name )
          ^4

    The Employee class isn't shown, but for this to work it must either
    have a conversion to string or a conversion ctor taking a string.  Both
    cases create a temporary object, invoking either operator= for strings
    or for Employees.  (There could be an operator= taking one of each
    which wouldn't require a temporary, but that's unlikely.)

    [Guideline] Watch out for hidden temporaries created by parameter
                conversions.  One good way to avoid this is to make
                ctors explicit when possible.


5. return "";
          ^5

    This creates a temporary (empty) string object.

    More subtly, it's better to declare a local string object to hold the
    return value and have a single return statement that returns that
    string.  This lets the compiler use the return value optimisation to
    omit the local object in some cases, e.g., when the client code writes
    something like:

        string a = FindAddr( l, "Harold" );

    [Rule]      Follow the single-entry/single-exit rule.  Never write
                multiple return statements in the same function.

*. string FindAddr( /*...*/ )
   ^^^*^^

    This was a red herring.  It may seem like you could avoid a temporary
    in all return cases simply by declaring the return type to be string&
    instead of string.  Wrong (generally see note below)!  If you're lucky,
    your program will crash as soon as the calling code tries to use the
    reference, since the (local!) object it refers to no longer exists.  If
    you're unlucky, your code will appear to work and fail intermittently,
    causing long nights of toiling away in the debugger.

    [Rule]      Never, ever, EVER return references to local objects.

    (Note: Some posters correctly pointed out that you could make this a
           reference return without changing the function's semantics by
           declaring a static object that is returned on failure.  This
           illustrates that you do have to be aware of object lifetimes
           when returning references.)


As some pointed out, there were other optimisation opportunities, such as
avoiding the redundant calls to end().  As others pointed out, the
programmer could/should have used a const_iterator.  Ignoring these for
now, a corrected version follows:

    string FindAddr( const list<Employee>& l, const string& name )
    {
        string addr;
        for( list<Employee>::const_iterator i = l.begin(); i != l.end();
++i )
        {
            if( (*i).name == name )
            {
                addr = (*i).addr;
                break;
            }
        }
        return addr;
    }