Practical PHP Refactoring: Replace Exception with Test

Sometimes catching an exception can be transformed in a preliminary check that avoid raising the exception in the first place. The code is then called only in the normal case, while the previously exceptional ones just terminate early te current method. So we're not talking about automated test but abount conditional inserted in the right place.

Why avoiding an exception?

We saw in the last article that exceptions are generally a more flexible solution with respect to error codes. However, they're not a cleaner solution with respect to everything else. For example, most of the flow control should be managed with ordinary code, as it is easier to understand than a sequence of try/catch blocks. Exceptions should be reserved to real errors.

Thus, when an exception can be easily avoided by a preliminary check, it is preferred to perform this test instead of calling a method while we can already perfectly know the call will fail.

Originally, this refactoring was also related to slow exception implementations. In reality, it's just about simplicity: why the hassle of throwing and catching objects for communication when a dedicated method call can do the same?

Steps

In the initial situation, the client code is calling a method on a server object.

1. Insert a conditional for the faulty case: it may have to delegate to the server object some of its logic.
2. Copy the code from the catch block into the if statement. Usually, it is throwing a new exception or returning a particular value.
3. The catch should now throw a base Exception object to check it is not executed in any covered case. Run the test suite.
4. Remove the catch, and even the try construct if there are no other exceptions to manage.

Example

This refactoring is rare with PHP native functions, which are still procedural. However, libraries like Zend Framework, Symfony and Doctrine have a rich exception hierarchy and can tempt a developer into trying and catching instead of trying to fully understand their API and behavior.

However, we will use one of the few PHP core object-oriented libraries, PDO, for the sake of the portability of this example.

This object is meant to hide PDO from the rest of the application: as such, it also throw its own exception instead of making the client code depend on and deal with PDOException objects. I'm modelling only the creation phase, omitting all the methods not relevant to this example:

<?php
class ReplaceExceptionWithTest extends PHPUnit_Framework_TestCase
{
    /**
     * @expectedException DatabaseException
     */
    public function testTheConnectionIsNotCreatedIfTheDriverIsNotSupported()
    {
        $database = new Database('somecloneofsqlite', ':memory:');
    }
}

class Database
{
    private $connection;

    public function __construct($driver, $restOfDsn)
    {
        try {
            $dsn = $driver . ':' . $restOfDsn;
            $this->connection = new PDO($dsn);
        } catch (PDOException $e) {
            throw new DatabaseException("The connection was not successful: check the configuration (dsn: '$dsn').");
        }
    }
}

class DatabaseException extends Exception {}

We copy the catch content, which throws another exception, into a guard clause.

class Database
{
    private $connection;
    private $supportedDrivers = array('sqlite', 'mysql');

    public function __construct($driver, $restOfDsn)
    {
        $dsn = $driver . ':' . $restOfDsn;
        if (!in_array($driver, $this->supportedDrivers)) {
            throw new DatabaseException("The connection was not successful: check the configuration (dsn: '$dsn').");
        }
        try {
            $this->connection = new PDO($dsn);
        } catch (PDOException $e) {
            throw new DatabaseException("The connection was not successful: check the configuration (dsn: '$dsn').");
        }
    }
}

To make sure the catch is not reached anymore, we throw a generic exception in it. PHPUnit will always signal a generic Exception as an error, even if we add it to @expectedException.

    public function __construct($driver, $restOfDsn)
    {
        $dsn = $driver . ':' . $restOfDsn;
        if (!in_array($driver, $this->supportedDrivers)) {
            throw new DatabaseException("The connection was not successful: check the configuration (dsn: '$dsn').");
        }
        try {
            $this->connection = new PDO($dsn);
        } catch (PDOException $e) {
            throw Exception();
        }
    }

Running the tests confirm the catch is unreachable code (for the cases covered by our test suite, of course):

[17:15:37][giorgio@Galen:~/Dropbox/practical-php-refactoring]$ phpunit ReplaceExceptionWithTest.php
PHPUnit 3.6.4 by Sebastian Bergmann.

.

Time: 1 second, Memory: 2.50Mb

OK (1 test, 1 assertion)

Now we can remove the try/catch block to finally clean up the code:

class Database
{
    private $connection;
    private $supportedDrivers = array('sqlite', 'mysql');

    public function __construct($driver, $restOfDsn)
    {
        $dsn = $driver . ':' . $restOfDsn;
        if (!in_array($driver, $this->supportedDrivers)) {
            throw new DatabaseException("The connection was not successful: check the configuration (dsn: '$dsn').");
        }
        $this->connection = new PDO($dsn);
    }
}

Other errors in the configuration may raise a PDOException. But it's beneficial to discover some PDOException object bubbling up and cover them with further test: the alternative is continue to raise a DatabaseException for all the cases, containing either a very generic or incorrect failure message.