The One True Iterator Declaration

The One True Iterator Declaration

Continueing my rich history of bikeshed-quality blog posts, I proudly present:

Three common ways to declare the iterators for iterating over a map:

  1. for ( map<string, string>::const_iterator it = map.begin(); it != map.end(); ++it );
    Not bad. Unfortunately the line is kinda long, and the .end() function is queried over and over again. On the plus side, the iterator is nicely scoped (only valid within the loop) – but that doesn’t work with MSVC6. Oh well.

  2. map<string, string>::const_iterator it = map.begin();
    map<string, string>::const_iterator end = map.end();
    for ( ; it != end; ++it );

    Not so cool. Both ‘it’ and ‘end’ outside of the scope of the loop. Might be what you want (for instance, the loop could break as soon as it (no pun intended) found a match. The super-sort for() line looks nice, and there are no unnecessary calls to map.end().

  3. map<string, string>::const_iterator it, end = map.end();
    for ( it = map.begin(); it != end; ++it );

    My personal style. The lines are both pretty short – in particular, I don’t have to type the silly map::const_iterator string twice. map.end() is queried only once.The only thing which bothers me (a little bit) is that ‘it’ and ‘end’ are valid outside of the loop, which is usually not what I want. I usually put the whole thing into { } to fix that.

How do you do it? 🙂

24 Comments

  1. Marijn Kruisselbrink 13 years ago

    What is wrong with something like
    for ( map::const_iterator it = map.begin(), end = map.end(); it != end; ++it );

  2. Author
    Frerich Raabe 13 years ago

    Yes, that is pretty nice, even though the line gets pretty long again (even longer than the first version I mentioned).

    I wish there was some nice trick to solve this all (and no, the foreach macro from Qt4 doesn’t cut it 🙂

  3. Luke Benstead 13 years ago

    Aha, but the STL also has a foreach and it’s not a macro 🙂

    I prefer this method:

    typedef map::const_iterator StringIter;
    for (StringIter=map.begin(); it != map.end(); ++it)

  4. Author
    Frerich Raabe 13 years ago

    stl::for_each requires a functor (or a function pointer). Unfortunately C++ does not have lambda functions, and I’d rather not move every loop body into a function of its own. :-}

  5. Luke Benstead 13 years ago

    Fair point, still they are handy when you want to iterate through objects and call a member function. My previous post was wrong, it should have read:

    typedef map::const_iterator StringIter;
    for (StringIter it =map.begin(); it != map.end(); ++it)

    but even still I just realized that means calling .end() each loop. So not as efficient as Marijn Kruisselbrink’s one. So this will help with the long line,

    typedef map::const_iterator StringIter;
    for (StringIter it =map.begin(), end = map.end(); it != end; ++it)

  6. Andreas 13 years ago

    Theoretically, if you are using a const iterator, comparing to const_end() should NOT result in a function call every iteration. If you only call const functions on the map, the compiler should know that function call results can be cached in a temporary.
    This only applies to collections that have a const_end() in the first place, of course, like Qt’s.
    I wonder if common compilers really “believe” in const and perform the appropriate optimizations…

  7. Luke Benstead 13 years ago

    Apparently they do, you reminded me of this thread:

    http://www.gamedev.net/community/forums/topic.asp?topic_id=230443&PageSize=25&WhichPage=1

    If you look on the second page they proved the compilers do perform optimizations when const is involved. It’s interesting reading.

  8. Pedro Alves 13 years ago

    I don’t get the fuss around long lines. You do know that you
    can have the for loop declared over several lines, right?

    for ( map::const_iterator it = map.begin(), end = map.end();
    it != end;
    ++it )
    {
    }

    There you go: No namespace clutter, and nicelly indented…

    How it is worse than your 3. ?

  9. Author
    Frerich Raabe 13 years ago

    Yes, I do. However – that’s matter of personal style – I try not to waste too many lines by having too much whitespace in them (so that I can have a good number of lines in my editor without sacrifying readability).

    When I do


    for ( map<string, string>::const_iterator it = map.begin(), end = map.end();
    it != end;
    ++it )
    {
    }

    Then all but the first line are ‘wasted’ in the sense that they contain very little code.

    It’s kinda hard to draw a line. I suspect my gut feeling comes down to ’30 out of 80
    characters per line should be real characters, no whitespace’.

  10. Pedro Alves 13 years ago

    There you go, then:

    for ( map::const_iterator
    it = map.begin(), end = map.end(); it != end; ++it ) {
    }

    Of course, this is all a matter of taste 🙂

    I tend to declare the iterators it/end before the loop myself.

    Happy coding!

    Cheers,
    Pedro Alves

  11. Tim 13 years ago

    I personally go for 1 because it is all on one line (who can’t fit that on their screen these days?)

    I kind of like 3, although the comma throws my mental parsing a bit – they are rarely used and can be a bit dangerous, e.g.

    int* a, b;

    is the same as

    int* a;
    int b;

    not

    int* a;
    int* b;

    as one might expect.

    And the space comes *after* the & or *! My opinion is the only valid one!

    Also surely the .end() is optimised out of the loop?

  12. Bart 13 years ago

    @Andreas : the need to evaluate end() really has nothing to do with const. The map could still change in the loop even with const iterators. Hack even if the map itself was const it could still change inside the for loop (and even without const-cast).

    Nevertheless I still prefer the first option if std::vector::end() is your bottleneck you shouldn’t probably not be using std::vector at all…

  13. Lee 13 years ago

    I was going to recommend the same thing as Marijn Kruisselbrink suggested. Since he’s already suggested it though, and you’re not satisfied, I’d go for combining that with the typedef suggestion:

    typedef map::const_iterator strmap_iter;

    for ( strmap_iter it = map.begin(), end = map.end(); it != end; ++it );

  14. Lee 13 years ago

    Or, rather:

    typedef map<string, string>::const_iterator strmap_iter;

    (I forgot to escape the angle brackets 🙂

  15. Esben Mose Hansen 13 years ago

    for_each, tranform, accumulate and copy + boost::lambda handles a lot of the common cases (and would probably handle more for me if I could wrap my head around them)

    I do sort of like the style that declares the end part in the first part of the for look. Never thought about that.

    Am I the only one who suspects that I have a harder time with the captchas than a good OCR program would? 😉

  16. Herbert Graeber 13 years ago

    Who says ther are no line breaks allowed inside the head of the for?
    So, what about

    for (
    map::const_iterator it = map.begin();
    it != map.end(); ++it
    ) {

    }

  17. Anonymous 13 years ago

    BOOST_FOREACH( std::map::value_type i, myMap ) {

    }

  18. ryan 13 years ago

    I prefer to put things in a transform, for_each, or something like that. THe ugly is that the function has to be declared outside. C++ could really use 1) lambda expressions and 2) inline function declarations. Functional programming techniques rule. And no, boost::lambda and FC++ do not suffice.

  19. Victor T. 13 years ago

    In all proposals till now I’m missing the fact that “end” could be declared constant:

    const map::const_iterator end = map.end();

    I tend to agree with Scott Meyers that everything that might be declared constant also should be declared constant. It might even help some (not-too-bright) compilers to generate better code. And obviously it also catches possible coding errors, as for instance a (wrong) assignment to “end” later on in the source code.

  20. Jason Voegele 13 years ago

    You must be very careful with style #3. If the loop body modifies the container it is possible that the iterator held in the end variable will have been invalidated and your program will likely crash. It is best not to use this style unless it is guaranteed that the loop body will not invalidate the iterator, and profiling indicates that it is worth performing this optimization (it probably is not in most circumstances).

    For this reason (among others) I prefer style #1, or using a standard algorithm if possible.

  21. Max Howell 13 years ago

    I tend to just go for all in one lines nowadays, since screens got bigger it’s acceptable IMO.

    But then again I hate huge variable declarations as they clutter the code and make parsing the code less quick by eye. Hence I never use the STL if I’m writing a Qt app 🙂

    In the above example I’d prolly use the two liner. But I find most people, including myself miss double definitions of variables in this form. Not the end of the world, but it can be frustrating when trying to locate a variable’s type. And KDevelop still doesn’t help with a tooltip there.

  22. jbo5112 13 years ago

    Sorry to dig up an old discussion, but this method is too cool! Of course you can skip the typedefs and name them manually (I’m just doing it here so my example is generic enough, easy to follow, easy to change and actually works). I can’t tell you the hours I’ve wasted trying to get something like this to work, and please note the first data type in the pair has to be const.

    using namespace std;

    typedef string map_key_type;
    typedef string map_value_type;

    map my_map;

    BOOST_FOREACH( pair &data, my_map )
    {
    // do your work on each pair here…
    }

  23. jbo5112 13 years ago

    okay, trying to do this without getting mangled (need a preview or edit button)…

    using namespace std;

    typedef string map_key_type;
    typedef string map_value_type;

    map<map_key_type, map_value_type> my_map;

    BOOST_FOREACH( pair<const map_key_type, map_value_type> &data, my_map )
    {
    // do your work on each pair here…
    }

  24. synss 13 years ago

    The BOOST_FOREACH thing is precisely what I was looking for, though to shorten the line and increase genericity

    BOOST_FOREACH(my_map::value_type &data, my_map)
    {
    /* … */
    }

    is better.

Leave a reply

Your email address will not be published. Required fields are marked *

*