Code That Should Not Exist but Does

The following snippets (slightly obscured for the sake of anonymity) are examples of code I have stumbled upon running in production in various systems. These are systems that sometimes handle financial transaction data and personal data. My fear is that this is much more common than we like to believe. These snippets were clearly written by someone new to not only PHP but programming in general. That, however, is not the problem! There is absolutely no shame in learning. The problem here is that that developer was actually tasked with writing this code as well as the blatantly obvious lack of code reviews.

deleteEntity()

/**
 * Delete entity with sub nodes
 * @param int $id
 */
public function deleteEntity($id = 0)
{
    $id = 123;

    $nodes = $this->EntityModel->find([
        "SELECT" => "id",
        "WHERE"  => "parent_id = $id",
    ]);

    foreach ($nodes as $node) {
        $this->EntityModel->delete($node['id']);
    }
}

Yes, you can pass an ID but it does not matter because it is always going to delete ID 123 because reasons. Incidentally this also erradicates the otherwise glaring SQL injection you get from passing $id directly into a WHERE clause.

At least there is a comment indicating what the function is supposed to be doing and what parameters it takes.

generateReferenceNo()

public function generateReferenceNo($id = 0, $entityNo)
{
    $reference_no  = $this->formatReferenceNo($id);
    $reference_no .= "-";

    if ($entityNo < 10) {
        $reference_no .= "00" . $entityNo;
    } elseif ($entityNo < 100) {
        $reference_no .= "0" . $entityNo;
    } else {
        $reference_no .= $entityNo;
    }

    return $reference_no;
}

Apart from the fact that the parameter ordering and optionality is backwards ($id is optional and defaults to 0 but you must supply an entity number thus making $id not optional) this is perhaps one of the more cumbersome ways I have ever seen left-padding done. Yes, it works—to some extent (unless you feed it anything that is not a positive integer)—but it can be replaced by sprintf("%s-%03d", $id, $entityNo); which will even handle negative numbers correctly.

In case you want to keep the poor handling of negative numbers and other unexpected input, PHP’s str_pad() can do that: str_pad($entityNo, 3, "0", STD_PAD_LEFT).

Conclusion

Again, this is not a dig at the developer(s) but rather at the organization that allowed this code to end up in production systems. I am certain that the developer solved the issue to the best of his/her ability and only did what s/he was asked.

Code That Should Not Exist but Does

2018/10/29

deleteEntity()

generateReferenceNo()

Conclusion