How to make foreach loops that don't suck

Calling values in Hashes/Maps/Sets

QHash<QString,Plasma::Meter*> meters;
foreach (Plasma::Meter *w, meters.values()) {
  w->setLabelColor(0, theme->color(Plasma::Theme::TextColor));
}


This code is iterating over the values of a hash and doing something over them. Everything seems ok, but it's not, it's calling values() on the hash and that's causing the creation of a temporary list with all the values of the list, effectively causing a bigger memory usage and iterating over the list twice (one to get the values and other to do something over them).

You can write something that does the same without these two problems doing

QHash<QString,Plasma::Meter*> meters;
foreach (Plasma::Meter *w, meters) {
  w->setLabelColor(0, theme->color(Plasma::Theme::TextColor));
}


So if you are using values() in a foreach, just remove it and your code will be instantaneously faster.

Calling keys in Hashes/Maps


QHash<QString,Plasma::SignalPlotter*> plotters;
foreach (const QString& key, plotters.keys()) {
  plotters.value(key)->setShowLabels(detail == SM::Applet::High);
  plotters.value(key)->setShowHorizontalLines(detail == SM::Applet::High);
}


This is calling keys() over the hash, this, as values(), means going across the container and creating a temporary list containing all the keys of the hash, that's a waste of memory when you could simply use a proper
QHash<QString,Plasma::SignalPlotter*>::const_iterator
and just iterate keys one by one.

Then it does plotters.value(key) twice, ok, accessing a value in a hash is amortized O(1) but still you are going to calculate the qHash of the string twice for no reason.

But more important, what the code is doing is actually iterate over the values, so the proper way is doing

QHash<QString,Plasma::SignalPlotter*> plotters;
foreach (Plasma::SignalPlotter *plotter, plotters) {
  plotter->setShowLabels(detail == SM::Applet::High);
  plotter->setShowHorizontalLines(detail == SM::Applet::High);
}


Probably in this case the net benefit is not much because probably the number of element of plotters is small, but there's no need to write inefficient code when the efficient one is as easy to write.

And of course it could be worse. If plotters was a map instead of a hash, lookup would be O(log(n)).

So if you are using keys() in a foreach, please think twice what you are doing and if you only need the value, iterate of the container and if you really need the key use a proper iterator to avoid constructing the temporary list.

Q_FOREACH is your friend

It's real, foreach makes writing loops much more convenient, but you have to know how to use it, see the following code:

#include <QList>
#include <QtDebug>

class Foo
{
 public:
  Foo()
  {
   qDebug("Empty constructor");
  }

  Foo(int aa, int bb, int cc) : a(aa), b(bb), c(cc)
  {
   qDebug("Constructor");
  }

  Foo(const Foo &f)
  {
   a = f.a;
   b = f.b;
   c = f.c;
   qDebug("Copy constructor");
  }

  int a, b, c;
};

int main(int /*argc*/, char ** /*argv*/)
{
 QList<Foo> list;
 list << Foo(1, 2, 3);
 list << Foo(4, 5, 6);
 list << Foo(7, 8, 9);
 list << Foo(10, 11, 12);
 list << Foo(13, 14, 15);

 foreach(Foo f, list)
  f.a = f.b = f.c = 0;

 foreach(Foo f, list)
  qDebug() << f.a << f.b << f.c;
}

There are two problems in the usage of foreach in this code.

The first problem is serious, it is trying to use foreach to modify the values of vector. The code compiles, but if you run it, you'll notice nothing was changed, why oh why?! may you ask. Because Foo f is a local variable to the foreach, so what you are doing is modifying a variable that exists in this very same line and nothing more.

The second problem is that you are constructing far much more Foo objects than needed, if you look at the output the copy constructor is called each time in the foreach, so the correct way for the debug line would be

 foreach(const Foo &f, list)
  qDebug() << f.a << f.b << f.c;
}

So remember, always that your foreach iterator is not a basic value like an int, double or pointer, use const &, it will be faster and will protect you against the try to use foreach to modify values problem, because

 foreach(const Foo &f, list)
  f.a = f.b = f.c = 0;

will not compile.