Tag: boolean

The Side Effect of Using Return Type To Handle Errors

I came across a curious little side effect of the issue I just posted earlier:

public function action()
{
   $id = filter_input(INPUT_POST, 'id', FILTER_SANITIZE_STRING);
	
   if(is_bool($id))
   {
      die('invalid id');
   }

   if(isset($_POST['accept']))
   {
      $this->acceptId($id);
   }
   else if(isset($_POST['reject']))
   {
      $this->rejectId($id);
   }
}

This is obviously going to do something when a POST has occured, but the method was invoked each and every time. The reason why it works? If there is no variable named ‘id’ POSTed, filter_input returns null. And null is not boolean, so the test passes. But as it’s not a POST request, neither of the two other if-tests are true, so the code silently passes through (id can contain characters here, so the filter isn’t used to just get integers).

If you’re not going to throw an error when the parameter is missing, the test is actually completely useless. This bug had hidden itself within the usage of the type to test for fault instead of actually checking the value, and did not creep out before I rewrote the if-test.

Communicating The Right Thing Through Code

While trying to fix a larger bug in a module I never had touched before, I came across the following code. While technically correct (it does what it’s supposed to do, and there is no way to currently get it to do something wrong (.. an update on just that), does have a serious flaw:

$result = get_entry($id);
if(is_bool($result))
{
    die('bailing out');
}

Hopefully you can see the error in what the code communicates; namely that the return type from the function is used to what should be considered an error.

While this works as the only way the function can return a boolean value is if it returns false, the person reading the code at a later date will wonder what the code is supposed to do – he or she might not have any knowledge about how the method works. Maybe the method just sets up some resource, a global variable (.. no, don’t do that. DON’T.), etc, but the code does not communicate what we really expect.

As PHP is dynamically typed, checking for type before comparing is perfectly OK, as long as you’re not counting “no returned elements” as an error. The following code more clearly communicates it intent (=== in PHP is comparison based on both type and content, which means that both the type of the variable and the content of it have to match. 0 === “0” will be considered false.):

$result = get_entry($id);
if($result === false)
{
    die('bailing out');
}

Or if you’re interested in getting to know if the element returned is actually considered false (such as an empty array, an empty string, etc), just drop one of the equal signs:

$result = get_entry($id);
if($result == false)
{
    die('bailing out');
}

I’m also not fond of using die() as a method for stopping a faulty request, as that should be properly logged and dealt with in the best manner possible, but I’ll leave that for a later post.