This is just a short shout out to the PHP folks - please do not cast variables during comparison using either ! or empty().
Let me explain with an example.
When I look at a piece of code like this (an example controller written Symfony-style):
declare(strict_types=1);
namespace App\Controller;
use External\Service\Api;
use Symfony\Component\HttpFoundation\Response;
final class OrderController
{
/**
* @var Api
**/
private $api;
public function __construct(Api $api)
{
$this->api = $api;
}
public function __invoke(): Response
{
$orders = $api->get('/orders');
if (!$orders) {
return new Response('No orders');
}
$this->processOrders($orders);
return new Response('Processed');
}
/**
* Oh no, there is no type hint for $orders.
*/
private function processOrders($orders): void
{
// Both array and strings can be iterated with a for loop.
// This approach is far from ideal, but not impossible to encounter.
// count() will throw an exception if the value is not countable,
// but only since version 7.2. Before that, a string will also work.
for ($i = 0; $i <= count($orders); $i++) {
$this->api->post('/complete-order', ['id' => $orders[$i]]);
}
}
}
then if I do not have previous experience with the code, I need to do additional checks to see what is actually going on here. Is $orders an array? Is it a JSON string? Something else? I would have to check the API class to see if it has a return type hint and if it does not, I would have to check the processOrders to see how it treats the $orders variable.
Let us say it usually returns an array of integers. But what if it would return something different, like a 0, due to some freak error? In that case, it will be cast to a false and the controller would not even notice the difference. But if, for example, you would get a valid JSON string, it would evaluate to true and processOrders will receive a string to iterate over, trying to use individual letters as order ids. It is a made up (and a bit silly) example, I agree, but completely in the realm of possibility, especially if it is part of a legacy application.
Now, we can rewrite it like so:
declare(strict_types=1);
namespace App\Controller;
use Exception;
use External\Service\Api;
use Symfony\Component\HttpFoundation\Response;
final class OrderController
{
/**
* @var Api
**/
private $api;
public function __construct(Api $api)
{
$this->api = $api;
}
public function __invoke(): Response
{
$orders = $api->get('/orders');
if (false === is_array($orders)) {
throw new Exception(
sprintf(
'Expected an array, got "%s".',
true === is_object($orders) ? get_class($orders) : gettype($orders)
)
);
}
if (0 === count($orders)) {
return new Response('No orders');
}
$this->processOrders($orders);
return new Response('Processed');
}
private function processOrders(array $orders): void
{
// By using array_walk(), we get additional type check for the
// individual array items.
array_walk($orders, function (int $id): void {
$this->api->post('/complete-order', ['id' => $id]);
});
}
}
... then you will still get an exception in case of an incorrect value being fetched from the API. So why bother?
Two reasons:
- You will not attempt to execute any code with data that is not at least of a correct type.
- Both you and anyone else looking at the code will have no additional mental overhead from figuring out what is being used and if there is any potential for error in the code. If you are not certain of the validity of the data being used, you may feel inclined to investigate and/or refactor a potential hazard.
Let me know your thoughts about this.
Cheers
Top comments (1)
More explicit checks make the code more readable and remove ambiguity early and not just pass the confusion down the stack 👍
Still, understanding how $api—>get() behaves is our first priority. If it’s unreliable, we may have to adapt it to something more reliable.