Emergency import occurred yesterday. X thousands of millions entries had to find their way into the database. I put on my superman lederhosen and started cranking. Two minutes after starting everything went completely bonkers. The damned import spewed out errors upon errors in the log. “Holy MAC-arel!” I shouted and spat violently on the floor(). Like a mental patient staring at the medicine cabinet craving for stronger drugs I read the code for the import routine. Curious about what I saw? Here’s a slightly rewritten “example” of how it “works”:

<!--?<span class="hiddenSpellError" pre="" data-mce-bogus="1"-->php
class GodObject {
    // Object setup functions omitted ...
    public function setAttributes($attr_1, $attr_2, ... ) {
        if(empty($attr_1)) { return null; }
        else if(empty($attr_2)) { return null; }
        else if( ... ) { return null; }
        else { /* Populating and storing goes here */ }
    }
}

// Driver file
<?php
    // list retrieval and alike omitted
    foreach($list as $line) {
        $object = new GodObject();

        if($object->setAttributes($line[0],$line[1], ...) === null)
        {
            error_log('something went wrong');
        }
    }
}

?>

These are two separate files, one using the another. The driver file is responsible of obtaining a multidimensional array and then loop over it and store whatever inside into an object which get stored in the database. The worst part was there wasn’t any PHPUnit tests available. Sigh. Anyhow – switching to my evil professor outfit I started to look into how to refactor this monster. Following are my approach, notes, ideas and thoughts on how to mend it:

(le petite change of focus happens between here and the next line)

Notes on GodObject class

The function ‘setAttributes’ takes a fixed amount of input parameters. Each of these are tested for empty values – if empty return ‘null’ to indicate error. The ‘else’ clause does the heavy lifting of insertion. Notice it doesn’t explicitly return anything.

Why is this a bad ‘pattern’?

  1. It will always return ‘null’. The If’s are obvious. PHP will return ‘null’ since there are no explicit return statement in ‘else’.
  2. Arguments are discarded if they seem empty. But integer value 0 and Boolean false would be trapped too. 
  3. Returning just ‘null’ in any cases are bad. Very bad. The caller isn’t informed what went wrong.
  4. There are no PHPDocBlock stating what the argument are, neither does it state what I can expect the function to return.

Notes on the driver file

The driver file is short – as it should be. It fetches an multi dimensional array and runs through it creating objects.

Why is this a bad ‘pattern’?

  1. The If fails since ‘setAttributes’ always return ‘null’. ‘Null’ is equal to Boolean False.
  2. Error message is worthless. It states something wrong happened – but not the cause of it.

Proposal of a better implementation

If I were to rewrite this import routine I would start with the GodObject class and

  1. create a public static function for validating input. This must return either True or False based on result. Static functions are great if we must use the validation service outside the class.
  2. create private class member for each attribute, each defaulting to ‘null’. Note: I assume ‘null’ is an illegal value to set from any caller for this.
  3. create public setters for each attribute. Each setter uses the static validation function. If the value sent in validates, set private class member for the attribute and return True. Otherwise throw an Exception stating what went wrong, which attribute we tried to set and what the value sent in actually was.
  4. create a public ‘save’ function. It should revalidate the attributes before saving using the static validation function. If this fails, throw an Exception. Otherwise continue saving throwing exceptions later in the saving process if any hick-ups occur. At the very end if everything is okay – return ‘true’.

Moving on to the driver I would

  1. place a try catch in the foreach loop.
  2. in catch block get hold of the exception message from GodObject, append information on wich line/object we are currently working on and then write that to the error log. Hopefully we know which entity caused the error ,which attribute failed and why it failed when reading the error log later on.
  3. inside the try block using the setters from GodObject set each attribute accordingly. Any exceptions thrown will be caught by the catch block from step 2.
  4. inside the try block call the object save function. Exceptions are caught by the catch block from step 2.

Final words

I haven’t refactored this code. Yet. But I am more or less likely be the person to do it. With this little run through I got an idea how things could/should work. I might have to apply some unit testing on this also. And PHPDocBlocks.

I know there are some errors and some things I kept out of this example. See if you can find them! 😉

Advertisements