All Over the Map

        Submitter: Omer Mussaev

        Language: C++

        Date:        2004/02/07

How often have you heard that every variable name should be meaningful? And that "i" is actually a bad name for anything? Well, there are worse.

Consider this code:

for (std::map<int,state>::const_iterator
theIterator = states.begin();
theIterator < states.end();
theIterator++) {
doSomething((*theIterator).second) ;
}

Now compare it to:
std::map<int,state>::const_iterator
    i(states.begin()), end(states.end());
for (; i != end; ++i ) {
doSomething(i->second) ;
}

Which is more readable?

Morale: If you feel that absolutely, positively have to use silly names, or if you just do not happen to be able to come with a meaningful alternative, then a) use shorter names and b) invest into other means of readablity.

Oleg Goldshmidt

 Omer is absolutely right, of course. However, readability is not the only thing that is wrong with the original code. Consider the original for loop. First, for each iteration the code calls states.end(), which returns a temporary object that is created and destroyed. This is very inefficient. Second, the loop uses postincremnent rather than preincrement. Think how the two differ: postincrement has to remember and return its previous value, thus creating another unneeded temporary 1,2,3

Now what can we conclude from the fact that Omer in fact corrected both problems without even mentioning them? One hypothesis is that experienced software engineers tend to write code that is readable, correct, and efficient at the same time, without thinking of these qualities separately. Another thought that I cannot get rid of easily is that when the code is readable and easily understood it tends to be correct and efficient --- there are no intrinsic tradeoffs here.

Another point that can be made is that it is usually worthwhile to reuse standard library algorithms and code without handcrafting loops and stuff like that. Consider

typedef std::pair<int,state> state_descriptor;
struct doSomething : public unary_function<state_descriptor, void>
{
void operator()(state_descriptor &s) { ... }
}
...
for_each(states.begin(),states.end(),doSomething());

--- no explicit iterators, no hidden temporaries, no preincrement/ postincrement dilemmas, everything is neat, correct, and efficient. 

-------------------------------------------

1 And you cannot hope that the compiler will optimize the postincrement: it is only allowed to do so for basic types such as int. For user-defined types the semantics of preincrement and postincrement may be very different. Of course, if you have such code you should submit it to this repository at once, but the compiler must deal with reality 2,3.

2 You can hope that the compiler will optimize postincrement for you if you implement it the usual way, via preincrement, and declare it inline. If the compiler respects the inline directive, it will see the unused temporary and optimize it away, probably issuing a warning along the way. Among other problems inlined functions introduce additional compile-time dependencies, which is a big issue in sizeable projects. Using preincrement in the first place is usually a much better idea.

3 For an in-depth discussion of closely related issues consult the real Guru, Herb Sutter, in his Guru of the Week column or the "Exceptional C++" book. I learned a lot from there.