Lessons learned from loadController(null)

Published on and tagged with cakephp  software engineering

Yesterday, I stumbled over the following code snippet in Cake and wondered what it does:

loadController(null);

If you read that statement, it simply doesn’t make sense. To load a controller with the name set to null, huch? A look at the API doesn’t bring any clarification:

Loads a controller and its helper libraries.
Parameters:
string   $name Name of controller 
Returns:
boolean Success

So we have to dive into the source code (which we fortunately have) to figure out what’s going on:

function loadController($name) {
    if (!class_exists('AppController')) {
        if (file_exists(APP . 'app_controller.php')) {
            require(APP . 'app_controller.php');
        } else {
            require(CAKE . 'app_controller.php');
        }
    }
    if ($name === null) {
        return true;
    }
    // load the controller
    ...
}

And we see that the AppController is loaded.

Why do I show this example here? Not to blame the developer, but because this example teaches us some lessons:

Listen to what your code tells you: By using (and allowing) “null” as parameter for the loadController function our code no longer tells us what it does. It doesn’t say “I load the AppController”, it says “I load the controller with null as name”, which is of course nonsense.

DRY (don’t repeat yourself) applies to API docs, too: If we compare the API docs for the loadController function with the function signature, we see that the function signature tells us the same as the doc comment, and is therefore redundant.

Mention unobvious stuff in the API docs: As we have seen, the obvious (and redundant) stuff is in the API docs, whereas the unobvious stuff (= loading the AppController when passing “null” as parameter) is not mentioned. So by looking at the API we can’t figure out the full functionality of the function…

Don’t do too much (in a single function): If we look at the source code of the loadController function we see that it does two different things: loading the AppController, and loading the specified controller. But as the function is about loading controllers (and not about loading the AppController), it should delegate the loading of the AppController to a loadAppController function.

7 comments baked

  • nate

    You’re more than welcome to update the docblock. ;-)

    That’s definitely one area where we could use some help.

  • Tarique Sani

    Good point – Comment the “why” of code rather than the much obvious “how” of code.

    Comments like //Increment $counter by 1 are very obvious and thus the comment is redundant but “why” the $counter is being incremented may not be obvious and needs to be commented

  • cakebaker

    @nate, Tarique: Thanks for your comments!

    @nate: For me the issue is not the incomplete doc block but the semantic of the function. But as discussed in IRC we disagree in this point.

    @Tarique: I fully agree with you.

  • Fallenjehova

    Nice post Daniel,

    I’m veeeeery curious. As the irc log is unavalaible right now, do you or nate have the irc logs about this topic? (semantics of this function).

  • cakebaker

    @Fallenjehova: Thanks. I’m sorry, but I don’t have the IRC logs.

    Well, the semantics of the function signature loadController($name) is that it loads the controller with the specified name. If you now pass null as parameter, you violate this semantic as null is not a valid controller name and hence the function should throw an error (and not load the AppController).

    I hope it is now more clear :)

  • m3nt0r

    Even its a little late: I agree with Daniel. I would never expect a function to do something if i pass “null”.

    In all coding guidelines i’ve read, people suggested to be verbose in naming so that the code is understandable without knowing the entire source.

    * hasSomething
    * doSomething
    * isSomething

    In the case described we “load” a “controller”. So if i would read a source that has the following in it, i would expect to happen or fail.

    if (loadController($userinput)) {
        // success
    } else {
       // notice user
    }

    Now let’s assume $userinput is not set, empty – it’s null. In that case the current loadController would return true, even though the userinput is not available and nothing happens as expected. The user did something wrong. He did not enter a controller name, but the function keeps returning “true”. That’s bad…

    I don’t know a way how to make this clearer.

    If i read API Docs and i see a bool-function with a descriptive name like “createFile” and just a single argument i expect it to fail if the value is left empty.

    When i write functions like that i begin with an if-sentence before i do anything else and i encourage everyone to do so too.

    if($name) 
       require app_controller, 
       require $name 
    (else) 
       error, return false

    Even “null” is there for a reason. :-)

    JM2C

  • cakebaker

    @m3ntor: Thanks for your comment, I agree fully with you ;-)

Bake a comment




(for code please use <code>...</code> [no escaping necessary])

© daniel hofstetter. Licensed under a Creative Commons License