Is Iterator initialization inside for loop considered bad style, and why?

Question

Typically you will find STL code like this:

for (SomeClass::SomeContainer::iterator Iter = m_SomeMemberContainerVar.begin(); Iter != m_SomeMemberContainerVar.end(); ++Iter)
{
}

But we actually have the recommendation to write it like this:

SomeClass::SomeContainer::iterator Iter = m_SomeMemberContainerVar.begin();
SomeClass::SomeContainer::iterator IterEnd = m_SomeMemberContainerVar.end();
for (; Iter != IterEnd; ++Iter)
{
}

If you're worried about scoping, add enclosing braces:

{
    SomeClass::SomeContainer::iterator Iter = m_SomeMemberContainerVar.begin();
    SomeClass::SomeContainer::iterator IterEnd = m_SomeMemberContainerVar.end();
    for (; Iter != IterEnd; ++Iter)
    {
    }
}

This is supposed to give a speed and efficiency gain, especially if you are programming consoles, because the .end() function is not called on each iteration of the loop. I just take the performance improvement for granted, it sounds reasonable but i don't know how much and it certainly depends on the type of container and actual STL implementation in use. But having used this style for a couple months now i actually prefer it over the first anyway.

The reason being readability: the for line is neat and tidy. With qualifiers and member variables in real production code it is quite easy to have really long for lines if you use the style in the first example. That's why i intentionally made it to have a horizontal scrollbar in this example, just so you see what i'm talking about. ;)

On the other hand, you suddenly introduce the Iter variables to the outer scope of the for loop. But then, at least in the environment i work in, the Iter would have been accessible in the outer scope even in the first example.

What is your take on this? Are there any pro's to the first style other than possibly limiting the scope of Iter?

Answer 1

The first form (inside the for loop) is better if the iterator is not needed after the for loop. It limits its scope to the for loop.

I seriously doubt that there is any efficiency gain either way. It can also be made more readable with a typedef.

typedef SomeClass::SomeContainer::iterator MyIter;

for (MyIter Iter = m_SomeMemberContainerVar.begin(); Iter != m_SomeMemberContainerVar.end(); ++Iter)
{
}

I would recommend shorter names ;-)

Answer 2

You can throw braces around the initialization and loop if you are concerned about scope. Often what I'll do is declare iterators at the start of the function and reuse them throughout the program.

Answer 3

I find the second option more readable, as you don't end up with one giant line. However, Ferruccio brings up a good point about the scope.

Answer 4

I agree with Ferruccio. The first style might be preferred by some in order to pull the end() call out of the loop.

I might also add that C++0x will actually make both versions much cleaner:

for (auto iter = container.begin(); iter != container.end(); ++iter)
{
   ...
}

auto iter = container.begin();
auto endIter = container.end();
for (; iter != endIter; ++iter)
{
   ...
}

Answer 5

I would usually write:

SomeClass::SomeContainer::iterator Iter = m_SomeMemberContainerVar.begin(),
                                   IterEnd = m_SomeMemberContainerVar.end();

for(...)

Answer 6

If you wrap your code into lines properly, the inline form would be equally readable. Besides, you should always do the iterEnd = container.end() as an optimization:

for (SomeClass::SomeContainer::iterator Iter = m_SomeMemberContainerVar.begin(),
    IterEnd = m_SomeMemberContainerVar.end();
    Iter != IterEnd;
    ++Iter)
{
}

Update: fixed the code per paercebal's advice.

Answer 7

I don't have a particularly strong opinion one way or the other, though iterator lifetime would lean me toward the for-scoped version.

However, readability may be an issue; that can be helped by using a typedef so the iterator type is a bit more manageable:

typedef SomeClass::SomeContainer::iterator sc_iter_t;

for (sc_iter_t Iter = m_SomeMemberContainerVar.begin(); Iter != m_SomeMemberContainerVar.end(); ++Iter)
{
}

Not a huge improvement, but a bit.

Answer 8

I don't have any console experience, but in most modern C++ compiliers either option ends up being equivilent except for the question of scope. The visual studio compilier will virtually always even in debug code put the condition comparison in an implicit temporary variable (usually a register). So while logically it looks like the end() call is being made through each iteration, the optimized compiled code actually only makes the call once and the comparison is the only thing that is done each subsiquent time through the loop.

This may not be the case on consoles, but you could unassemble the loop to check to see if the optimization is taking place. If it is, then you can you whatever style you prefer or is standard in your organization.

Answer 9

Another alternative is to use a foreach macro, for example boost foreach:

BOOST_FOREACH( ContainedType item, m_SomeMemberContainerVar )
{
   mangle( item );
}

I know macros are discouraged in modern c++, but until the auto keyword is widely available this is the best way I've found to get something that is concise and readable, and still completely typesafe and fast. You can implement your macro using whichever initialization style gets you better performance.

There's also a note on the linked page about redefining BOOST_FOREACH as foreach to avoid the annoying all caps.

Answer 10

It may make for disjointed code, but I also like to pull it out to a separate function, and pass both iterators to it.

doStuff(coll.begin(), coll.end())

and have..

template<typename InIt>
void doStuff(InIt first, InIt last)
{
   for (InIt curr = first; curr!= last; ++curr)
   {
       // Do stuff
   }
 }

Things to like:

• Never have to mention the ugly iterator type (or think about whether it's const or not-const)
• If there is gain from not calling end() on each iteration, I'm getting it

Things to not like:

• Breaks up the code
• Overhead of additional function call.

But one day, we'll have lambdas!

Answer 11

No, it's a bad idea to get a hold on iter.end() before the loop starts. If your loop changes the container then the end iterator may be invalidated. Also, the end() method is guaranteed to be O(1).

Premature optimization is the root of all evil.

Also, the compiler may be smarter than you think.

Answer 12

Having looked at this in g++ at -O2 optimisation (just to be specific)

There is no difference in the generated code for std::vector, std::list and std::map (and friends). There is a tiny overhead with std::deque.

So in general, from a performance viewpoint it makes little difference.

Answer 13

I don't think it's bad style at all. Just use typedefs to avoid the STL verbosity and long lines.

typedef set<Apple> AppleSet;
typedef AppleSet::iterator  AppleIter;
AppleSet  apples;

for (AppleIter it = apples.begin (); it != apples.end (); ++it)
{
   ...
}

Spartan Programming is one way to mitigate your style concerns.