Over the years, I’ve seen a bunch of coding anti-patterns. I figured maybe I’ll share a few.
Today, I’ll introduce what I’m calling the unrolled-switch anti-pattern, also known as “Specialization is always faster, right?”
enum Axis { XAxis, YAxis, ZAxis, }; // code earlier in the function ensure that // "axis" is always a valid axis int newPosition; switch (axis) { case XAxis: newPosition = m_position[XAxis] + amount; if (newPosition < m_minPosition[XAxis]) newPosition = m_minPosition[XAxis]; if (newPosition > m_maxPosition[XAxis]) newPosition = m_maxPosition[XAxis]; m_position[XAxis] = amount; break; case YAxis: newPosition = m_position[YAxis] + amount; if (newPosition < m_minPosition[YAxis]) newPosition = m_minPosition[YAxis]; if (newPosition > m_maxPosition[YAxis]) newPosition = m_maxPosition[YAxis]; m_position[YAxis] = amount; break; case ZAxis: newPosition = m_position[ZAxis] + amount; if (newPosition < m_minPosition[ZAxis]) newPosition = m_minPosition[ZAxis]; if (newPosition > m_maxPosition[XAxis]) newPosition = m_maxPosition[XAxis]; m_position[ZAxis] = amount; break; }
As we all know, special-case code is faster than general-purpose code. Instead of writing slow general-purpose code:
newPosition = m_position[axis] + amount; if (newPosition < m_minPosition[axis]) newPosition = m_minPosition[axis]; if (newPosition > m_maxPosition[axis]) newPosition = m_maxPosition[axis]; m_position[axis] = amount;
we unroll it into a switch statement, thereby generating highly-optimized special-purpose code, one for each axis.
What makes this anti-pattern particularly frustrating is that you cannot tell at a glance whether all the cases really are the same (just with different axes).
In fact, they aren’t.
If you look closely, you’ll see that we check the new Z-position against the X-axis maximum rather than the Z-axis maximum. If you’re reading this code, you now start to wonder, “Is this a copy/paste bug, or is there some reason that we really do want to check the Z-position against the X-axis minimum?”
A variation on the unrolled-switch is the unrolled-if, used if the item you want to unroll cannot be used in a switch statement:
FruitBasket *BananaBasket; FruitBasket *AppleBasket; FruitBasket *PearBasket; FruitBasket *MangoBasket; if (basket == BananaBasket) { if (!BananaBasket->IsEmpty()) { fruit = BananaBasket->TakeFruit(); if (HaveKnife()) { TakeKnife(); fruit->Peel(); fruit->Slice(); fruit->Eat(); ReplaceKnife(); } else { BananaBasket->AddFruit(fruit); } } } else if (basket == AppleBasket) { if (!AppleBasket->IsEmpty()) { fruit = AppleBasket->TakeFruit(); if (HaveKnife()) { TakeKnife(); fruit->Peel(); fruit->Slice(); fruit->Eat(); ReplaceKnife(); } else { AppleBasket->AddFruit(fruit); } } } else if (basket == PearBasket) { if (!PearBasket->IsEmpty()) { fruit = PearBasket->TakeFruit(); if (HaveKnife()) { TakeKnife(); fruit->Slice(); fruit->Eat(); ReplaceKnife(); } else { PearBasket->AddFruit(fruit); } } } else if (basket == MangoBasket) { if (!MangoBasket->IsEmpty()) { fruit = MangoBasket->TakeFruit(); if (HaveKnife()) { TakeKnife(); fruit->Peel(); fruit->Slice(); fruit->Eat(); ReplaceKnife(); } else { BananaBasket->AddFruit(fruit); } } }
When I pointed out in an aside to the customer that this could be simplified (after fixing the copy/paste errors) to
if (!basket->IsEmpty()) { fruit = basket->TakeFruit(); if (HaveKnife()) { TakeKnife(); fruit->Peel(); fruit->Slice(); fruit->Eat(); ReplaceKnife(); } else { basket->AddFruit(fruit); } }
the response was, “Hey, that’s a neat trick. I didn’t realize you could do that.”
I wonder if this person also programs loops like this:
switch (limit) { case 0: break; case 1: do_something(array[0]); break; case 2: for (int i = 0; i < 2; i++) do_something(array[i]); break; case 3: for (int i = 0; i < 3; i++) do_something(array[i]); break; case 4: for (int i = 0; i < 4; i++) do_something(array[i]); break; case 5: for (int i = 0; i < 5; i++) do_something(array[i]); break; case 6: for (int i = 0; i < 6; i++) do_something(array[i]); break; ... case 999: for (int i = 0; i < 999; i++) do_something(array[i]); break; default: FatalError("Need more cases to handle larger array"); break; }
0 comments