I ran into this funny from Twitter on Reddit a day or two ago.
A quick Code Review. (found on Reddit – yeah, I may be spending too much time on Reddit lately.
Here is the code we are looking at. The request is to simplify it while maintaining the original functionality.
function user_is_premium() {
// if premium is false, we immediately return false
if ($_GET['premium'] === 'false') return false;
// param 1, cookie 1 -> true
if ($_GET['premium'] === 'true' && $_COOKIE['premium'] !== 'true') return true;
// param 1, cookie 0 -> true
if ($_GET['premium'] === 'true' && $_COOKIE['premium'] === 'true') return true;
// param 0, cookie 1 -> true
if ($_GET['premium'] !== 'true' && $_COOKIE['premium'] === 'true') return true;
// param 0, cookie 0 -> false
if ($_GET['premium'] !== 'true' && $_COOKIE['premium'] !== 'true') return false;
}
// Example of use:
if (user_is_premium()) {
echo "You are awesome";
}
else
{
echo "Did you know you can become a Premium member?";
}If it looks like a duck and quacks like a duck it should be a duck.
This is in php. The GET and Cookie parameters are passed as strings — but the evaluations here are Boolean-ish (true or false). There are string comparisons that are looking for an exact match (===) on the strings ‘true’ and ‘false’. I dislike this because I think it looks confusing. When I see true and false I want those to be Boolean assignments unless there is a good reason for them not to be.
PHP provides variable filters that are really useful when handling forms or API calls. So we can get rid of the string matching right of the bat by using filter_var with the filter ‘FILTER_VALIDATE_BOOLEAN’. this takes the variable and tests it for truthy and falsey values and returns the proper value.
So the evaluations can be done like this : filter_var( $_GET[‘premium’], FILTER_VALIDATE_BOOLEAN); which will return true or false as expected.
Code that contradicts itself
You might have noticed that the first line of code in the function returns false if the GET input is ‘False’. This is good – if a function is going to have a ‘failed’ state, testing for that first and then returning the false value and not executing the rest of the function is performant.
The problem in this function is that after we have bailed out of the function the GET is evaluated for false again – this time if it is ‘false’ it will look at the COOKIE and return true or false based on the string value of the COOKIE. But that line of code is never evaluated. Elvis has already left the building at that point.
There are two contradictory logics – the function returns false if the GET is ‘false’ but then it has code to return true if the GET is ‘false’ but the COOKIE is ‘true’.
This is where we need clarity
Which way is really the desired output? I would suspect that since the four cases are explicitly coded out that they are what is really wanted, but really — I am not sure.
Since there are only two options I will go ahead and refactor it twice.
First option is the “if the GET is false it is always false ignore the cookie” option.
So in this case I will get rid of the function all together and just return the result of filter_var
if (filter_var( $_GET['premium'], FILTER_VALIDATE_BOOLEAN)) {
echo "You are awesome";
}
else
{
echo "Did you know you can become a Premium member?";
}The other option is to return true if the GET or the COOKIE or both are true. If I know that I will never want to do anything else but know if the case is true or false then I would update the function to just do this:
function user_is_premium() {
return filter_var( $_GET['premium'], FILTER_VALIDATE_BOOLEAN) || filter_var( $_COOKIE['premium'], FILTER_VALIDATE_BOOLEAN);
}But if I suspected that I might want to extent this function and call other functions or return something more than true or false I would use a ternary:
function user_is_premium() {return (filter_var( $_GET['premium'], FILTER_VALIDATE_BOOLEAN) ? true : (filter_var( $_COOKIE['premium'], FILTER_VALIDATE_BOOLEAN) ? true : false))
}