Unnecessary temporaries are frequent culprits that can throw all your hard work - and your program's performance - right out the window.
Problem
You are doing a code review. A programmer has written the following function, which uses unnecessary temporary objects in at least three places.
How many can you identify, and how should the programmer fix them?
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 "";
}
Solution
Believe it or not, these few lines have three obvious cases of unnecessary temporaries, two subtler ones, and a red herring.
string FindAddr( list<Employee> l, string name )
^^^^^^^1^^^^^^^^ ^^^^^2^^^^^
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.
for( list<Employee>::iterator i = l.begin();
i != l.end();
i++ )
^3^
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.
if( *i == name )
^4
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. (The only way there wouldn't be a temporary is if there was an operator= taking one of each.)
[Guideline] Watch out for hidden temporaries created by parameter conversions. One good way to avoid this is to make ctors explicit when possible.
return "";
^5
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.
[Note: After more performance testing, I no longer agree with the above advice. It has been revised in Exceptional C++.]
string FindAddr( list<Employee> l, string name )
^^^*^^
*. 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.)
There are other optimisation opportunities, such as avoiding the redundant calls to end(). The programmer could/should also 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;
}
