Type checking vs too much typing…

This had me pondering for a minute.

Quite often I write code along the lines of:

public function doStuff($stuff, $complex = FALSE) {
  if ($complex) {
      echo 'Doing complex stuff<br />';
  } else {
      echo 'Doing simple stuff<br />';
  }
}

Alrighty, this does not seem too bad.
If our default is $complex = FALSE, then we’ll always be doing “simple stuff”.

However, once the client (developer) accesses the method based on the given API … We could have a potential problem.

Let’s look at these scenarios:

$this->doStuff('some stuff'); //lazy developer, knows the API, doesn't bother with unnecessary argument
$this->doStuff('some stuff', FALSE); //proper developer, better be safe than sorry
$this->doStuff('some stuff', 'false'); //silly developer

As you can imagine in the first two cases we were “Doing simple stuff”… However because the third developer accidentally (or what have ya) enclosed the ‘false’ Boolean into a string PHP took care of type conversion and had to evaluate that string to meaning TRUE.

PHP’s fault? …. neah
Developer’s fault? …. neah (although some will argue that).
Lazy API developer? … most likely.

Let’s improve the method a little:

public function doStuff($stuff, $complex = FALSE) {
if (is_bool($complex) && $complex) {
     echo 'Doing complex stuff<br />';
  } else if (is_bool($complex) && !$complex) {
     echo 'Doing simple stuff<br />';
  } else {
     echo 'You have passed the wrong type, silly<br />';
  }
}

More, but safer code. Just wanted to point this out. Pick your own battles.

  • Pingback: Tweets that mention Type checking vs too much typing… | nuts and bolts of cakephp -- Topsy.com

  • Nick

    “Developer’s fault? …. neah (although some will argue that).”

    Indeed, I would argue that. ‘false’ != FALSE in ANY language. If the API calls for a boolean, how is passing a string NOT the developer’s fault?

  • Helio Ricardo

    Oops,

    public function doStuff($stuff, $complex = FALSE) {
    echo (bool($complex))? ‘Doing complex stuff': ‘Doing simple stuff';
    }

    Best Reg,
    Hélio

    • teknoid

      @Helio Ricardo

      Excellent solution, but slightly not aligned with the point of the post.
      It actually would work in a similar manner to the original function().

      also you’ve probably meant (bool) $complex

  • http://kevin.vanzonneveld.net kvz

    What happened to $complex === false?

  • kleingeist

    i have to absolutely agree on Nick’s comment: the api says boolean, if you give it something different the value of the function/method doesn’t have to be defined. so it’s the developers fault if it wont work as he hopes.
    still i agree, that for public apis it makes sense to throw an exception (which btw. an language which is strict on types would do itself – we could make an php vx. xyz war about this)

  • http://jamienay.com Jamie

    I agree with Nick – it’s the developer’s fault. If a developer doesn’t know the difference between FALSE and ‘false’ then that developer will probably have problems with ANY code.

  • http://dogmatic69.com dogmatic69

    Instead of the is_bool() && checks, $complex === true, $complex === false would do the trick.

    another way is to just do the if(!is_boo($var)){return false;} in the start. this will make the code a bit cleaner also

  • teknoid

    @All
    Interesting points. Which side do I stand on? Neither…
    The goal was to give a few pointers that when writing some code, it’s best to be safe than sorry.
    The problem is that these types of bugs become very hard to track down especially late at night :) If your classes (API) is bullet-proof, then less worry “about” silly developers (guess, what… I’ll admit I can be one even after coupleteen years of doing this). And if you dig some in the core, cake is very forgiving of things like that.

    Regarding, type checking operands… they are certainly better to use, but not much clearer to explain the point at hand.

  • http://cakebaker.42dh.com/ Daniel Hofstetter

    I agree with the other commenters about it being the API user’s fault.

    However, I think boolean parameters are a code/design smell in most cases and should be avoided. Splitting the method into two methods doSimpleStuff() and doComplexStuff() leads to cleaner code and avoids the mentioned issue completely.

  • http://bachowski.pl Michał Bachowski

    Bit more code, but much more readable:

    public function doStuff($stuff, $complex = FALSE) {
    if ( !is_bool($complex) ) {
    echo ‘You have passed the wrong type, silly';
    return
    }
    if (!$complex) {
    echo ‘Doing simple stuff';
    } else {
    echo ‘You have passed the wrong type, silly';
    }
    }

    Regards

  • http://www.le6o.com le6o

    I agree with Michal, always best to return early. Although I would throw an InvalidArgumentException and be done with their silliness.

  • teknoid

    @All
    Thanks for your comments. The point I was really trying to make is to say that one should take care of protecting their API, rather than leaving it up for the developer to make poor choices.
    The saying “don’t trust the client”, applies in more ways than one… Imagine if PHP or Cake for that matter didn’t throw helpful hints along the way? Life would be much harder and we know that.

    … As far as code itself, well it wasn’t meant to be some holy grail of programming brilliance, just enough to clearly demonstrate alternative. Thanks for all the hints along the way, nonetheless.

    Cheers!

  • http://www.dereuromark.de mark

    actually i would leave it as it is… if ($complex) {} else {}
    thing is, in php we are not type safe.
    so most times we get a tinyint(1) value from the database, like “display_box” yes/no?

    this way – without a big fuzz – both this database field (of course you would want to check on the existence first) and any boolean value will work here.
    and any string will cause unexpected results – but that’s the developer to blame for here then.

    some could argue: well, cast it to (bool). we get the same unexpected results (because casting strings results in true here).
    anyway: it’s best to leave it simple (jesus, your “solution” is more complicated than all simple solutions together) and understandable.

    @param boolean also means that a “cakephp boolean tinyint(1)” is a valid param (because this pseudo-boolean value behaves like the real thing). result === expected result and everything’s fine^^.

    and yes – (string)”false” is so wrong that it should not even be considered :)
    otherwise you open Pandora’s box if you want to cover all possible (and sometimes really stupid) mistakes there are.

  • teknoid

    @mark

    I agree with you, but what happens if the request comes from an external post (API post or otherwise), and there’s no proper type checking?
    Better be safe than sorry, IMO.

    Like I said, pick your own battles ;)

  • http://www.phally.com Phally

    If only type hinting in PHP were any good. As it is implemented now, it is basically useless. You can use it for objects and arrays, but not for primary datatypes like the boolean from your example. If (or when) this is properly implemented I think the problem above can be handled a lot cleaner and you have less to worry about when programming.