Some time ago, Charles shared with us some awful PHP, aka the most common sort. Today's code sample is maybe a little too big to sum up, but I'll let Charles take a crack at it.
It's so bad that even analyzing and laughing at it feels impossible. But it’s so bad, I couldn’t not share it.
I’m the only one handling all the IT-related tasks at my company, and I don’t have anyone here to vent or laugh about this kind of thing with. So, I figured, why not share it here? I’m hoping it’ll provide at least a little bit of catharsis or some dark humor.
To make sure the confidentiality of the codebase was respected, I took the liberty of generalizing it. You might notice some inconsistencies, but that’s just me trying to keep things neutral while protecting the original structure and functionality. Apologies if it looks a bit patchy – the goal was to avoid revealing any specific details or sensitive code.
The whole block is north of 400 lines, and it's doing a lot. Or well, maybe it's not, as you'll see.
Let's star with the outermost layer.
$resm_data = $data_source->fetchData("group=" . $item_id);
foreach ($resm_data as $key => $value) {
// rest of the code here
}
We fetch data from a data source, presumably a database, passing our condition as a string, which reeks of probable SQL injection, but I don't know what library they're using. I also note they're using the key/value style of array iteration, but never actually check the key.
$option_id = $value->option_id;
$resm_details = $detail_source->fetch($option_id);
if ($resm_details) {
$label = $resm_details->{"label$lang"};
$description = $resm_details->{"description$lang"};
$category = $resm_details->category;
Nice little bit of "meta" programming to get their localization working, it'll fetch labelen or labelde as needed. Definitely not a horrible, dangerous way to solve that problem.
We use that again to get our currency figured out. That lets us do number formatting. So much number formatting code.
if ($category == 0) {
$cost = $resm_details->{"cost" . $currency};
$child_cost = $resm_details->{"child_cost" . $currency};
$cost_info =
$mot[101] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol . " - " .
$mot[102] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
} elseif ($category == 1) {
$cost = $resm_details->{"cost" . $currency};
$child_cost = 0;
$cost_info = $mot[101] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
} elseif ($category == 2) {
$cost = 0;
$child_cost = $resm_details->{"child_cost" . $currency};
$cost_info = $mot[102] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
} elseif ($category == 4) {
$cost = $resm_details->{"cost" . $currency};
$child_cost = 0;
$cost_info = $mot[500] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
} elseif (
$resm_details->{"cost" . $currency} == 0 and
$resm_details->{"child_cost" . $currency} == 0
) {
$cost = 0;
$child_cost = 0;
$cost_info = "";
} else {
$cost = $resm_details->{"cost" . $currency};
$child_cost = 0;
$cost_info = $mot[200] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
}
What is the $mot array? Why am I jumping to seemingly random locations in that array? Clearly it contains some headers for our output.
Anyway, there's plenty of HTML string munging happening too, don't you worry.
if ($location == 1) {
$quantity_block =
'<div class="quantity-container">
<input type="text" value="1" id="quantity-' . $option_id . '" class="qty-control" name="quantity" min="1" max="1">
<div class="increase button">+</div>
<div class="decrease button">-</div>
</div>';
$quantity = 1;
} else {
$quantity_block = "";
$quantity = "all";
}
And then there's this little treat for parsing the time stored in our database: $time_data = json_decode($resm_details->time_data); That tells me they're storing date times as strings, so that's fun.
There are also a couple more bon mots as they build a drop down list:
$departure_select = '<option value="-1">' . $mot[300] . '</option>';
$arrival_select = '<option value="-1">' . $mot[301] . '</option>';
And then there's this monstrosity:
// Add the main option to the template
$TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS", [
"ID" => $option_id,
"QUANTITY" => $quantity,
"LABEL_CLASS" => $label_class,
"ACTION_CLASS" => $action_class,
"LABEL" => $label,
"DESCRIPTION" => $description,
"QUANTITY_BLOCK" => $quantity_block,
"COST_INFO" => $cost_info,
"HOST_COST_DISPLAY" => $host_cost_display,
"COST" => $cost,
"CHILD_COST" => $child_cost,
"LOCATION" => $location,
"CATEGORY" => $category,
"HOST_CLASS" => $host_class,
"EXTRA" => $extra,
"HOST_EXTRA" => $host_extra,
"TIME_CHECKED" => ($has_times == 1) ? 'selected="' . $option_id . '" checked="true"' : '',
"TIME_BLOCK" => $time_block
]);
All the nonsense that we concatenate together above gets shoved into some sort of template. And after that, that's where the good stuff starts. Because guess what? We have to do the same thing for child items.
$resm_children_data = $data_source->fetchData("parent=" . $option_id);
foreach ($resm_children_data as $child_key => $child_value) {
$child_option_id = $child_value->option_id;
$resm_child_details = $detail_source->fetch($child_option_id);
That's right, it's the same block of code, not quite copy/pasted, since they needed to put the word child in everything.
// Add the child option to the template
$TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS.Block_OPTIONS_CHILD", [
"ID" => $child_option_id,
"QUANTITY" => $child_quantity,
"LABEL_CLASS" => $child_label_class,
"ACTION_CLASS" => $child_action_class,
"LABEL" => $child_label,
"DESCRIPTION" => $child_description,
"QUANTITY_BLOCK" => $child_quantity_block,
"COST_INFO" => $child_cost_info,
"HOST_COST_DISPLAY" => $child_host_cost_display,
"COST" => $child_cost,
"CHILD_COST" => $child_extra_cost,
"LOCATION" => $child_location,
"CATEGORY" => $child_category,
"HOST_CLASS" => $child_host_class,
"EXTRA" => $child_extra,
"HOST_EXTRA" => $child_host_extra,
"TIME_CHECKED" => ($child_has_times == 1) ? 'selected="' . $child_option_id . '" checked="true"' : '',
"TIME_BLOCK" => $child_time_block
]);
And now, if you liked the child record, guess what? Those children have got siblings. What can I say, it's a big family.
$resm_sibling_data = $data_source->fetchData("parent=" . $parent_option_id);
foreach ($resm_sibling_data as $sibling_key => $sibling_value) {
$sibling_option_id = $sibling_value->option_id;
$resm_sibling_details = $detail_source->fetch($sibling_option_id);
And that means, yes, we also use that template thing again:
// Add the sibling option to the template
$TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS.Block_OPTIONS_CHILD", [
"ID" => $sibling_option_id,
"QUANTITY" => $sibling_quantity,
"LABEL_CLASS" => $sibling_label_class,
"ACTION_CLASS" => $sibling_action_class,
"LABEL" => $sibling_label,
"DESCRIPTION" => $sibling_description,
"QUANTITY_BLOCK" => $sibling_quantity_block,
"COST_INFO" => $sibling_cost_info,
"HOST_COST_DISPLAY" => $sibling_host_cost_display,
"COST" => $sibling_cost,
"SIBLING_COST" => $sibling_extra_cost,
"LOCATION" => $sibling_location,
"CATEGORY" => $sibling_category,
"HOST_CLASS" => $sibling_host_class,
"EXTRA" => $sibling_extra,
"HOST_EXTRA" => $sibling_host_extra,
"TIME_CHECKED" => ($sibling_has_times == 1) ? 'selected="' . $sibling_option_id . '" checked="true"' : '',
"TIME_BLOCK" => $sibling_time_block
]);
Someday, I hope the person who wrote this learn about methods and function calls. Maybe they could write their own one day.
In any case, here's the whole thing:
$resm_data = $data_source->fetchData("group=" . $item_id);
foreach ($resm_data as $key => $value) {
$option_id = $value->option_id;
$resm_details = $detail_source->fetch($option_id);
if ($resm_details) {
$label = $resm_details->{"label$lang"};
$description = $resm_details->{"description$lang"};
$category = $resm_details->category;
// Process cost based on category
if ($category == 0) {
$cost = $resm_details->{"cost" . $currency};
$child_cost = $resm_details->{"child_cost" . $currency};
$cost_info =
$mot[101] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol . " - " .
$mot[102] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
} elseif ($category == 1) {
$cost = $resm_details->{"cost" . $currency};
$child_cost = 0;
$cost_info = $mot[101] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
} elseif ($category == 2) {
$cost = 0;
$child_cost = $resm_details->{"child_cost" . $currency};
$cost_info = $mot[102] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
} elseif ($category == 4) {
$cost = $resm_details->{"cost" . $currency};
$child_cost = 0;
$cost_info = $mot[500] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
} elseif (
$resm_details->{"cost" . $currency} == 0 and
$resm_details->{"child_cost" . $currency} == 0
) {
$cost = 0;
$child_cost = 0;
$cost_info = "";
} else {
$cost = $resm_details->{"cost" . $currency};
$child_cost = 0;
$cost_info = $mot[200] . " : +" . number_format($cost, 2, ",", " ") . $currency_symbol;
}
$location = $resm_details->location;
$label_class = $location == 1 ? "has-quantity" : "no-quantity";
$action_class = $location == 1 ? "active-with-quantity" : "active-no-quantity";
if ($location == 1) {
$quantity_block =
'<div class="quantity-container">
<input type="text" value="1" id="quantity-' . $option_id . '" class="qty-control" name="quantity" min="1" max="1">
<div class="increase button">+</div>
<div class="decrease button">-</div>
</div>';
$quantity = 1;
} else {
$quantity_block = "";
$quantity = "all";
}
$has_times = $resm_details->has_times;
if ($has_times == 1) {
$time_counter++;
$time_data = json_decode($resm_details->time_data);
$departure_select = '<option value="-1">' . $mot[300] . '</option>';
$arrival_select = '<option value="-1">' . $mot[301] . '</option>';
$departures = $time_data->departures;
$arrivals = $time_data->arrivals;
foreach ($departures as $dep_key => $departure) {
$departure_select .= '<option value="' . $dep_key . '">' . $departure . '</option>';
}
foreach ($arrivals as $arr_key => $arrival) {
$arrival_select .= '<option value="' . $arr_key . '">' . $arrival . '</option>';
}
$departure_block = '<div class="col-half time-select-' . $option_id . '" style="padding-right: 0;">
<select class="form-control time-departure" style="text-align: center;">' . $departure_select . '</select>
</div>';
$arrival_block = '<div class="col-half time-select-' . $option_id . '" style="padding-left: 0;">
<select class="form-control time-arrival" style="text-align: center;">' . $arrival_select . '</select>
</div>';
$time_block = '<div class="row time-container">
' . $departure_block . $arrival_block . '
</div><small class="error-message time-error">' . $mot[302] . '</small>';
} else {
$time_block = '';
}
$host_cost_display = "";
$extra = $resm_details->extra;
$host_extra = $resm_details->host_extra;
$host_class = "";
if ($extra == 1) {
$extra_cost_1 = $resm_details->{"cost" . $currency . "_1"};
$extra_cost_2 = $resm_details->{"cost" . $currency . "_2"};
$default_extra = $host_price == 0 ? $extra_cost_2 : $extra_cost_1;
$cost_info = $mot[101] . ' : +<span class="extra-cost">' . number_format($default_extra, 2, ",", " ") .
"</span>" . $currency_symbol . " - " . $mot[102] . ' : +<span class="child-cost">0.00</span>' . $currency_symbol;
$host_class = " extra-option";
}
if ($host_extra == 1) {
$host_cost_display =
'<span class="host-cost">' . $mot[500] . ' : +<span class="host-price">0.00</span>' . $currency_symbol .
"</span><br>";
}
// Add the main option to the template
$TEMPLATE->SET_BLOCK_VARIABLES("Block_OPTIONS", [
"ID" => $option_id,
"QUANTITY" => $quantity,
"LABEL_CLASS" => $label_class,
"ACTION_CLASS" => $action_class,
"LABEL" => $label,
"DESCRIPTION" => $description,
"QUANTITY_BLOCK" => $quantity_block,
"COST_INFO" => $cost_info,
"HOST_COST_DISPLAY" => $host_cost_display,
"COST" => $cost,
"CHILD_COST" => $child_cost,
"LOCATION" => $location,
"CATEGORY" => $category,
"HOST_CLASS" => $host_class,
"EXTRA" => $extra,
"HOST_EXTRA" => $host_extra,
"TIME_CHECKED" => ($has_times == 1) ? 'selected="' . $option_id . '" checked="true"' : '',
"TIME_BLOCK" => $time_block
]);
$resm_children_data = $data_source->fetchData("parent=" . $option_id);
foreach ($resm_children_data as $child_key => $child_value) {
$child_option_id = $child_value->option_id;
$resm_child_details = $detail_source->fetch($child_option_id);
if ($resm_child_details) {
$child_label = $resm_child_details->{"label$lang"};
$child_description = $resm_child_details->{"description$lang"};
$child_category = $resm_child_details->category;
// Process cost for child category
if ($child_category == 0) {
$child_cost = $resm_child_details->{"cost" . $currency};
$child_extra_cost = $resm_child_details->{"child_cost" . $currency};
$child_cost_info =
$mot[101] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol . " - " .
$mot[102] . " : +" . number_format($child_extra_cost, 2, ",", " ") . $currency_symbol;
} elseif ($child_category == 1) {
$child_cost = $resm_child_details->{"cost" . $currency};
$child_extra_cost = 0;
$child_cost_info = $mot[101] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
} elseif ($child_category == 2) {
$child_cost = 0;
$child_extra_cost = $resm_child_details->{"child_cost" . $currency};
$child_cost_info = $mot[102] . " : +" . number_format($child_extra_cost, 2, ",", " ") . $currency_symbol;
} elseif ($child_category == 4) {
$child_cost = $resm_child_details->{"cost" . $currency};
$child_extra_cost = 0;
$child_cost_info = $mot[500] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
} elseif (
$resm_child_details->{"cost" . $currency} == 0 and
$resm_child_details->{"child_cost" . $currency} == 0
) {
$child_cost = 0;
$child_extra_cost = 0;
$child_cost_info = "";
} else {
$child_cost = $resm_child_details->{"cost" . $currency};
$child_extra_cost = 0;
$child_cost_info = $mot[200] . " : +" . number_format($child_cost, 2, ",", " ") . $currency_symbol;
}
$child_location = $resm_child_details->location;
$child_label_class = $child_location == 1 ? "has-quantity" : "no-quantity";
$child_action_class = $child_location == 1 ? "active-with-quantity" : "active-no-quantity";
if ($child_location == 1) {
$child_quantity_block =
'<div class="quantity-container">
<input type="text" value="1" id="child-quantity-' . $child_option_id . '" class="qty-control" name="quantity" min="1" max="1">
<div class="increase button">+</div>
<div class="decrease button">-</div>
</div>';
$child_q