PHP:CSI - To Switch, Or Not To Switch?

I was writing unit tests for a API mapping function recently and came across this interesting issue. The code I was writing tests for was in a legacy codebase that I was making changes to, and it made sense to have some unit tests in there before I started work to ensure everything worked before and after.

The mapping function in question would take a value as input and return another value as the output. The code seemed like it should work, but then applying certain values to the mapping function it would produce an incorrect result.

The mapping function was required to translate a true value to 1, a false value to 0, and default everything else to 1. I have removed a lot of other code to just show the section in question here, but the function looked a little like this.

function map_value($var) {
  switch ($var) {
    case TRUE:
      return 1;
    case FALSE:
      return 0;
    default:
      return 1;
  }
}

At face value, this seemed fine, but the specification I was working with required that any null values would be returned as 1. When I created unit tests to check for the different values that could be sent to this function it was clear that there was a problem.

Rather than adding the unit tests here I have added a simple example of the results that the function produces. Whilst true and false values produce the correct result, null values produce the incorrect result of false.

var_dump(map_value(TRUE) === 1); // true
var_dump(map_value(FALSE) === 0); // true
var_dump(map_value(NULL) === 1); // false

My initial attempt at a fix was to add null as a comparison to the switch statement so that when this value was passed to the function it would return the right result. This changed the function to the following.

function map_value($var) {
  switch ($var) {
    case TRUE:
    case NULL:
      return 1;
    case FALSE:
      return 0;
    default:
      return 1;
  }
}

When running the tests again, however, it was realised that this caused more problems than it solved. Although null was now returning 1, but a value of false was now returning a value of 1 as well. This is evidenced by the following code.

var_dump(map_value(TRUE) === 1); // true
var_dump(map_value(FALSE) === 0); // false
var_dump(map_value(NULL) === 1); // true

Some of you might have already guessed what was wrong here, but what was happening was that the switch statement was performing a loose comparison on the value. This loose comparison was essentially causing null to be seen as a false value, which meant that any false values were being seen as the same as null and therefore triggering the first check.

We can prove type coercion between null and false by looking at the outcome of casting a null value to be a boolean. The following example shows this and will print false.

var_dump((bool) NULL); // false

The PHP documentation about type comparison tables also shows this. Look under the section about "loose comparisons with ==" and you can see that a null value will be cast to a false when compared like this.

This effect is quite subtle, and I don't think I would have spotted it if null values returned 0. In fact, the test would have been correct and so there would have been no need to alter the code.

The Solution

Ultimately, the solution to this was to re-write the function to change the comparison from a weak comparison to a strong comparison. Essentially, by removing the switch statement we can perform a strict comparison using the === operator and be absolutely sure that the value is false before returning 0.

<?php

function map_value($var) {
  if ($var === FALSE) {
      return 0;
  }
  return 1;
}

Running the test again with this change allowed all of the values to pass correctly.

var_dump(map_value(TRUE) == 1); // true
var_dump(map_value(FALSE) == 0); // true
var_dump(map_value(NULL) == 1); // true

The lesson here is that switch statements are great when you are comparing strings or numbers as there is no type coercion going on. For anything else they can be the cause of subtle problems that you wouldn't have picked up until it was too late.

Let's say that instead of values we were accepting strings to the original function. If this was the case then we wouldn't have had to change the code at all as everything would have worked very well. We could have added the string 'null' to the comparison to return 1, but since the default behaviour of the switch is to return 1 then this line is mostly redundant.

function map_value($var) {
  switch ($var) {
    case 'true':
    case 'null':
      return 1;
    case 'false':
      return 0;
    default:
      return 1;
  }
}

Running the same comparisons, but this time using strings, proves that everything works correctly.

var_dump(map_value('true') === 1); // true
var_dump(map_value('false') === 0); // true
var_dump(map_value('null') === 1); // true

This is an arbitrary example, and you should still avoid writing code like this, but it shows a different approach that uses a switch statement whilst allowing for loose comparisons.

This also shows the power of unit test as without writing the unit test to test the mapping functions this error might have gone unnoticed until it was producing an unwanted effect.