There is some code, that at first glance, doesn’t seem great, but doesn’t leap out as a WTF. Stephe sends one such block.
double SomeClass::getMaxKeyValue(std::vector<double> list)
{
double max = 0;
for (int i = 0; i < list.size(); i++) {
if (list[i] > max) {
max = list[i];
}
}
return max;
}
This isn’t great code. Naming a vector
-type variable list
is itself pretty confusing, the parameter should be marked as const
to cut down on copy operations, and there’s an obvious potential bug: what happens if the input is nothing but negative values? You’ll incorrectly return 0
, every time.
Still, this code, taken on its own, isn’t a WTF. We need more background.
First off, what this code doesn’t tell you is that we’re looking at a case of the parallel arrays anti-pattern. The list
parameter might be something different depending on which key is being searched. As you can imagine, this creates spaghettified, difficult to maintain code. Code that performed terribly. Really terribly. Like “it must have crashed, no wait, no, the screen updated, no wait it crashed again, wait, it’s…” terrible.
Why was it so terrible? Well, for starters, the inputs to getMaxKeyValue
were often arrays containing millions of elements. This method was called hundreds of times throughout the code, mostly inside of window redrawing code. All of that adds up to a craptacular application, but there’s one last, very important detail which brings this up to full WTF:
The inputs were already sorted in ascending order.
With a few minor changes, like taking advantage of the sorted vectors, Stephe to the 0.03333 frames-per-second performance up to something acceptable.