CodeSOD: Currency Format

“Dark Horse” inherited some PHP code. They had a hundred lines to submit, but only sent in a dozen- which is fine, as the dozen lines tell us what the other hundred look like.

$suite_1_1 = number_format($item -> {‘suite_1_1_’.$the_currency}, 2, ‘.’, ”);
$suite_1_2 = number_format($item -> {‘suite_1_2_’.$the_currency}, 2, ‘.’, ”);
$suite_1_3 = number_format($item -> {‘suite_1_3_’.$the_currency}, 2, ‘.’, ”);
$suite_1_4 = number_format($item -> {‘suite_1_4_’.$the_currency}, 2, ‘.’, ”);

$suite_2_1 = number_format($item -> {‘suite_2_1_’.$the_currency}, 2, ‘.’, ”);
$suite_2_2 = number_format($item -> {‘suite_2_2_’.$the_currency}, 2, ‘.’, ”);
$suite_2_3 = number_format($item -> {‘suite_2_3_’.$the_currency}, 2, ‘.’, ”);
$suite_2_4 = number_format($item -> {‘suite_2_4_’.$the_currency}, 2, ‘.’, ”);

$suite_3_1 = number_format($item -> {‘suite_3_1_’.$the_currency}, 2, ‘.’, ”);
$suite_3_2 = number_format($item -> {‘suite_3_2_’.$the_currency}, 2, ‘.’, ”);
$suite_3_3 = number_format($item -> {‘suite_3_3_’.$the_currency}, 2, ‘.’, ”);
$suite_3_4 = number_format($item -> {‘suite_3_4_’.$the_currency}, 2, ‘.’, ”);

On one level, they have an object called $item, and want to format a series of fields to two decimal places. Their approach to doing this is to just… write a line of code for each one. But this code is so much worse than that.

Let’s start with the object, which has fields named in a pattern, suite_1_1_USD, and suite_2_1_EUR. Which right off the bat, why do we have so many fields in an object? What are we going to do with this gigantic pile of variables?

Now, because this object has values for different currencies, we need to ensure we only work on a single currency. They do this by dynamically constructing the field name with a variable, $the_currency. The code $item -> {“some” . “field”} is a property accessor for, well, “somefield”.

On one hand, I hate the dynamic field access to begin with, as obviously this all should be organized differently. On the other, I’m frustrated that they didn’t go the next logical step and loop across the two numeric fields. This whole mess would still be a mess, but it’d be a short mess.

All these currency values, and nobody thought to buy an array or two.

Source: Read More