5
\$\begingroup\$

I'm trying to improve the following code for testability.

public function can_apply_extension()
{

    $search_dates = [
        Carbon::createFromTimestamp(strtotime($this->billing_verified_to))->subMonth(1),
        $this->billing_verified_to
    ];

    $record = ORM::factory('a model')
                 ->where('created_at', 'BETWEEN', $search_dates)
                 ->find();

    if( !$record->loaded() ) 
    {
        return true;
    }

    return false;

}

Here's how I think I can improve it:

public function can_apply_extension($search_from = null, $search_to = null)
{

    $search_from = is_null($search_from) ? Carbon::createFromTimestamp(strtotime($this->billing_verified_to))->subMonth(1) : $search_from;
    $search_to = is_null($search_to) ? $this->billing_verified_to : $search_to

    $search_dates = [$search_from, $search_to];

    $record = ORM::factory('a model')
                 ->where('created_at', 'BETWEEN', $search_dates)
                 ->find();

    if( !$record->loaded() ) 
    {
        return true;
    }

    return false;

}

I still have to mock ORM part. I think the only option is that I pass an object as a parameter or make a method that injects the ORM object.

Are there any other better ways? I'm aware of IoC container, but I can't use it in our current environment.

Update: Beside passing ORM as a parameter so that I can just swap it out, I found another workaround.

Since ORM::factory() actually returns an object, I was able to add a method and modify the factory method.

public function testing($bool)
{ 
    $this->_testing = $bool;
}

public function instead($when_this_passed, ORM $use_this_instead)
{
    $this->_instead_map[$when_this_passed] = $use_this_instead;
}

public function factory($name)
{
    if($this->_testing && array_key_exists($name, $this->_instead_map))
    {
        return $this->_instead_map[$name];
    } 
    else
    {
        parent::factory($name);
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Get rid of all the static calls, because an ORM::factory implies a static property that holds a reference to an object. Testing classes with static properties is hard... really hard \$\endgroup\$ Commented Dec 20, 2013 at 15:55
  • \$\begingroup\$ @EliasVanOotegem // Thank you for the input. As I mentioned, that's one of the options. Are there any others? \$\endgroup\$
    – Moon
    Commented Dec 20, 2013 at 18:33

1 Answer 1

3
\$\begingroup\$

(I've not coded in PHP recently, my examples are Java or Java-like pseudocode.)

In his Working Effectively with Legacy Code book Michael C. Feathers describes a lot of techniques which help making any code testable. Another good resource is the following article: Killing the Helper class, part two

The usual way to breaking static dependencies is the following: Create a non-static interface which has a similar method than the original static class:

public interface OrmFactory {
    OrmSomething create($name);
}

Then create an implementation which calls the static method/class:

public class OrmFactoryImpl implements OrmFactory {
    public OrmSomething create($name) {
        return ORM::factory($name);
    }
}

Finally, use the interface in the can_apply_extension function:

public class MyClass {

    private OrmFactory ormFactory;

    public MyClass(OrmFactory ormFactory) {
        this.ormFactory = ormFactory;
    }

    public void can_apply_extension() {
        ...
        $record = ormFactory.create('a model')
                     ->where('created_at', 'BETWEEN', $search_dates)
                     ->find();
        ...
    }
}

In tests you can pass a mocked OrmFactory to the class while in production code you can use the original one through the OrmFactoryImpl wrapper/delegate. (I guess PHP also has some mocking framework implementations which makes testing easier but you can implement a TestOrmFactory which emulates some behaviour of the original one without the need of any database.)

The second snippet in the question has test logic in production smell. I would avoid that and use the much cleaner dependency breaking above. The linked page contains some disadvantages of the smell. One of them is the following:

Code that was not designed to work in production and which has not been verified to work properly in the production environment could accidently be run in production and cause serious problems.

And another one is:

Software that is added to the SUT For Tests Only makes the SUT more complex. It can confuse potential clients of the software's interface by introducing additional methods that are not intended to be used by any code other than the tests. This code may have been tested only in very specific circumstances and might not work in the typical usage patterns used by real clients.

(SUT means System Under Test, in this case the class with can_apply_extension().)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.