From assumptions to assertions

In your journey as a developer you not only develop applications, you also develop yourself. You and I both know there's more to developing than writing code that works, it's writing good code that works well. If you're like me then you probably started developing websites from scratch, later decided you wanted to build your own CMS and so on. In that stage as a developer you are usually very focused on getting stuff to work and not really on the unhappy paths.

As you continue to write code and continue to develop yourself you will eventually find yourself writing code like this, keep an eye on the is not null checks:

$lastOrder = null;

if ($user->getOrders()->count() > 0) {
    $orders = $this->filterByCompleted($user->getOrders());
    $lastOrder = $orders->last();
}

if ($lastOrder !== null) {
    if ($order->getBillingAddress() === null) {
        $order->setBillingAddress($lastOrder->getBillingAddress());
    }

    if ($order->getShippingAddress() === null) {
        $order->setShippingAddress($lastOrder->getShippingAddress());
    }
}

In the code above we retrieve the last order and use some information from there if it's missing in our current order (prefilling). Code like this is quite common and is sprinkled through most codebases I've worked with and contributed to. The problem with this code is that we're still making assumptions instead of assertions, just like we did back when we didn't check for unhappy paths at all and assumed all was good. The big cause of this are the is not null checks, they're a habit some of us have and it's not doing us as much good as we think it's doing.

Just to give you some insight, the $orders variable is of type Doctrine\Common\Collections\ArrayCollection and the last() functionactually uses PHP's end() function, if we check the documentation on that we can see that end() returns false when the array is empty. This may be something you already knew but this is not necessarily something you think about everytime you write code. With this in mind we can see that our code is flawed, it will run the if $lastOrder is not null check, pass and it will try to run $lastOrder->getBillingAddress() while $lastOrder is set to false. Our assumption was: "if $lastOrder is not null, it must be an object of type Order", just like we're assuming that $order->getBillingAddress()'s default value is null.

This brings me to the point of blacklisting vs whitelisting, I strongly believe we should be whitelisting in this case to turn assumptions into proper assertions. The above code could be rewritten as:

if ($lastOrder instanceof OrderInterface) {
    if (!$order->getBillingAddress() instanceof AddressInterface) {
        $order->setBillingAddress($lastOrder->getBillingAddress());
    }

    if (!$order->getShippingAddress() instanceof AddressInterface) {
        $order->setShippingAddress($lastOrder->getShippingAddress());
    }
}