Refactoring code with TDD

December 11, 2014

Where I test the factories make the function mocker play along with data providers.

Testing the Time factory

The code of the WPSchedule_Time_Factory has been written already and I have to test it to make sure a degree of robustness is there. Generating an empty test case takes just one command

$ codecept generate:phpunit unit WPSchedule_Time_Factory

and the tests/unit/WPSchedule_Time_FactoryTest.php file will be created stubbing out an empty test. I know the class will rely on WordPress defined get_option function and hence will add function-mocker to the test case with a use statement and calling its setUp and tearDown methods

use tad\FunctionMocker\FunctionMocker;

class WPSchedule_Time_FactoryTest extends \PHPUnit_Framework_TestCase {

    protected function setUp() {
        FunctionMocker::setUp();
    }

    protected function tearDown() {
        FunctionMocker::tearDown();
    }
}

With an empty test case I want to assert that the factory will return an instance of the WPSchedule_Time_Now class if the option is not set for the hook

/**
 * @test
 * it should return Now time if option not set
 */
public function it_should_return_now_time_if_option_not_set() {
    FunctionMocker::replace( 'get_option', false );

    $time = WPSchedule_Time_Factory::make( 'some_hook' );

    $this->assertInstanceOf( 'WPSchedule_Time_Now', $time );
}

I also want the factory to return an instance of the WPSchedule_Time_Now class in the case a non legit value has been stored in the option: to make my life easier I will be using a data provider method to simulate different plausible bad values

public function notATimeSlug() {
    return [
        [ 23 ],
        [ 'foo' ],
        [ [ ] ],
        [ [ 'one' => 1 ] ],
        [ [ 'one' => 1, 'two' => 'two' ] ]
    ];
}

/**
 * @test
 * it should return Now time by default
 * @dataProvider notATimeSlug
 */
public function it_should_return_now_time_by_default( $timeSlug ) {
    FunctionMocker::replace( 'get_option', $timeSlug );

    $time = WPSchedule_Time_Factory::make( 'some_hook' );

    $this->assertInstanceOf( 'WPSchedule_Time_Now', $time );
}

And finally want to test that instances of the proper class are returned for legit option values

public function legitTimeSlugs() {
    return [
        [ 'now', 'WPSchedule_Time_Now' ],
        [ '8pm', 'WPSchedule_Time_EightPM' ],
        [ '8am', 'WPSchedule_Time_EightAM' ]
    ];
}

/**
 * @test
 * it should return right time for slug
 * @dataProvider legitTimeSlugs
 */
public function it_should_return_right_time_for_slug( $timeSlug, $class ) {
    FunctionMocker::replace( 'get_option', $timeSlug );

    $time = WPSchedule_Time_Factory::make( 'some_hook' );

    $this->assertInstanceOf( $class, $time );
}

Running the tests again this code

class WPSchedule_Time_Factory implements WPSchedule_Interface_FactoryInterface {

    public static function make( $hook, array $args = null ) {
        if ( ! is_string( $hook ) ) {
            throw new Exception( 'Hook name must be a string' );
        }

        $timeSlug = get_option( $hook . '_schedule_time', 'now' );
        $timeSlug = is_string( $timeSlug ) ? $timeSlug : 'now';

        $instance = null;

        switch ( $timeSlug ) {
            case '8pm':
                $instance = new WPSchedule_Time_EightPM();
                break;
            case '8am':
                $instance = new WPSchedule_Time_EightAM();
                break;
            case 'now':
                $instance = new WPSchedule_Time_Now();
                break;
            default:
                $instance = new WPSchedule_Time_Now();
                break;
        }

        return $instance;
    }
}

will see the code pass [ WPSchedule_Time_Factory tests passing ](http://theaveragedev.local/wordpress/wp-content/uploads/2014/12/2014-12-11-at-17.38.png)

Something is wrong

The inherent problem in the test above, the last one specifically, is that:

knowledge specific to the class is duplicated in the test code (slug to class relation)
if I add a new slug/class couple the test code will have to be updated

Test code is code. Specifically it's code using a class as a client. What the two bullets above tell me is that, the same way I will have to update my test code as the WPSchedule_Time_Factory adds more cases, I will have to do the same for any other client code using the class.
That might be a necessary evil sometimes but not always: if test code can automatically keep up to date with a class under test as it evolves than client code will do too and modification to client (and test) code will not be needed.
Furthermore client code should not need knowledge of an object inner workings. That's true to a point here since I know the get_option function is being called internally but the whole idea of dependency injection is that knowledge of an object interface (as in public methods) should be enough to test and use it.

Automating

Easy to spot is the possibility to modify the test code and the class code to hide the knowledge of the slug and class couples either hidden from outside or clearly available via a public method: I choose the second option and will add a method to the WPSchedule_Time_Factory class

    public static function getSlugsAndClasses() {
        return array(
            'now' => 'WPSchedule_Time_Now',
            '8pm' => 'WPSchedule_Time_EightPM',
            '8am' => 'WPSchedule_Time_EightAM'
        );
    }

that will be used in the test with some manipulation due to the data provider method expected return value format

public function legitTimeSlugs() {
    $pairs = WPSchedule_Time_Factory::getSlugsAndClasses();
    $out = [ ];
    array_walk( $pairs, function ( $class, $slug ) use ( &$out ) {
        $out[] = [ $slug, $class ];
    } );

    return $out;
}

Refactoring - part 1

Once tests are in place and green I can refactor the class to my heart's content and will use the just added method to do it, running the tests on each step will tell me if my refactoring process broke something along the way.
A minute and 4 test runs later the WPSchedule_Time_Factory class looks like this

    class WPSchedule_Time_Factory implements WPSchedule_Interface_FactoryInterface {

    protected static $slugsAndClasses = array(
        'now' => 'WPSchedule_Time_Now',
        '8pm' => 'WPSchedule_Time_EightPM',
        '8am' => 'WPSchedule_Time_EightAM'
    );

    const OPTION_POSTFIX = '_schedule_time';

    protected static $defaultSlug = 'now';

    public static function make( $hook, array $args = null ) {
        if ( ! is_string( $hook ) ) {
            throw new Exception( 'Hook name must be a string' );
        }

        $timeSlug = get_option( $hook . self::OPTION_POSTFIX, self::$defaultSlug );
        $timeSlug = self::isLegitSlug( $timeSlug ) ? $timeSlug : self::$defaultSlug;

        $instance = null;

        $slugsAndClasses = self::getSlugsAndClasses();
        $class = $slugsAndClasses[ $timeSlug ];

        return new $class;
    }

    public static function getSlugsAndClasses() {
        return self::$slugsAndClasses;
    }

    public static function isLegitSlug( $timeSlug ) {
        $slugsAndClasses = self::getSlugsAndClasses();

        return is_string( $timeSlug ) && array_key_exists( $timeSlug, $slugsAndClasses );
    }
}

and tests are still all green.

Refactoring is not stopping here and I will move into another refactoring level entirely which is quite evident from the code above. The code ending this post is on GitHub tagged version 0.1.6.

Testing the Time factory

Something is wrong

Automating

Refactoring - part 1

Next