Refactoring the Cat API client - Part II

Posted on by Matthias Noback

The world is not a safe thing to depend upon

When you're running unit tests, you don't want the world itself to be involved. Executing actual database queries, making actual HTTP requests, writing actual files, none of this is desirable: it will make your tests very slow as well as unpredictable. If the server you're sending requests to is down, or responds in unexpected ways, your unit test will fail for the wrong reasons. A unit test should only fail if your code doesn't do what it's supposed to do.

As we saw in the previous part of this series, both the CachedCatApi and the RealCatApi classes are in some way dependent on the world. The first writes actual files to the filesystem, the second makes actual HTTP requests. Besides, creating files and making HTTP requests is pretty low-level stuff, for which these classes currently don't use the best tools available. Also, a lot of edge-cases have not been taken care of.

Both classes can be freed from their dependence on the world, simply by letting some new classes encapsulate the low-level details. For example, we can easily move the call to file_get_contents() to another class, called FileGetContentsHttpClient:

class FileGetContentsHttpClient
{
    public function get($url)
    {
        return @file_get_contents($url);
    }
}

Dependency inversion again

Just like in the previous article, extracting some code into a new class won't suffice. We need to introduce an interface for the new class, or we won't be able to easily create a stand in for it in our tests:

interface HttpClient
{
    /**
     * @return string|false Response body
     */
    public function get($url);
}

Now we can provide an HttpClient as a constructor argument of RealCatApi:

class RealCatApi implements CatAPi
{
    private $httpClient;

    public function __construct(HttpClient $httpClient)
    {
        $this->httpClient = $httpClient;
    }

    public function getRandomImage()
    {
        $responseXml = $this->httpClient->get('https://thecatapi.com/api/images/get?format=xml&type=jpg');

        ...
    }
}

A true unit test

For the first time we can now have a true unit test for RealCatApi. We only need to provide a stand-in for HttpClient which just returns a predefined XML response body:

class RealCatApiTest extends \PHPUnit_Framework_TestCase
{
    /** @test */
    public function it_fetches_a_random_url_of_a_cat_gif()
    {
        $xmlResponse = <<<EOD
<response>
    <data>
        <images>
            <image>
                <url>https://24.media.tumblr.com/tumblr_lgg3m9tRY41qgnva2o1_500.jpg</url>
                <id>bie</id>
                <source_url>https://thecatapi.com/?id=bie</source_url>
            </image>
        </images>
    </data>
</response>
EOD;
        $httpClient = $this->getMock('HttpClient');
        $httpClient
            ->expect($this->once())
            ->method('get')
            ->with('https://thecatapi.com/api/images/get?format=xml&type=jpg')
            ->will($this->returnValue($xmlResponse));
        $catApi = new RealCatApi($httpClient);

        $url = $catApi->getRandomImage();

        $this->assertSame(
            'https://24.media.tumblr.com/tumblr_lgg3m9tRY41qgnva2o1_500.jpg',
            $url
        );
    }
}

We have a proper test now, which verifies the correctness of RealCatApi's behavior: it should call the HttpClient with a specific URL and return just the value of the <url> element from the XML response body.

Decoupling our API from file_get_contents()

One issue still remains to be fixed right now: the contract of HttpClient::get() still adheres to the contract of file_get_contents() in that it returns false when the request failed, or the response body (as a string) when it succeeded. We can nicely hide this implementation detail by converting any specific return value like false into a custom exception. This way, the only things that travel across the object boundary are a function argument, a string return value, or an exception:

class FileGetContentsHttpClient implements HttpClient
{
    public function get($url)
    {
        $response = @file_get_contents($url);
        if ($response === false) {
            throw new HttpRequestFailed();
        }

        return $response;
    }
}

interface HttpClient
{
    /**
     * @return string Response body
     * @throws HttpRequestFailed
     */
    public function get($url);
}

class HttpRequestFailed extends \RuntimeException
{
}

We only need to modify RealCatApi a bit to catch the exception, instead of responding to false:

class RealCatApi implements CatAPi
{
    public function getRandomImage()
    {
        try {
            $responseXml = $this->httpClient->get('https://thecatapi.com/api/images/get?format=xml&type=jpg');

            ...
        } catch (HttpRequestFailed $exception) {
            return 'https://cdn.my-cool-website.com/default.jpg';
        }

        ...
    }
}

Did you notice that previously we only had a unit test for the happy path? We only tested the result when file_get_contents() gave us a proper XML response body. We weren't in the position to test for a failing HTTP request since, well, how would you "reliably" cause an HTTP request to fail, except by unplugging your network cable?

Since we now have full control over the HttpClient, we can simulate a failing HTTP request, simply by throwing an HttpRequestFailed exception:

class RealCatApiTest extends \PHPUnit_Framework_TestCase
{
    ...

    /** @test */
    public function it_returns_a_default_url_when_the_http_request_fails()
    {
        $httpClient = $this->getMock('HttpClient');
        $httpClient
            ->expect($this->once())
            ->method('get')
            ->with('https://thecatapi.com/api/images/get?format=xml&type=jpg')
            ->will($this->throwException(new HttpRequestFailed());
        $catApi = new RealCatApi($httpClient);

        $url = $catApi->getRandomImage();

        $this->assertSame(
            'https://cdn.my-cool-website.com/default.jpg',
            $url
        );
    }
}

Removing the filesystem dependency

We can repeat the same steps for CachedCatApi's dependency on the filesystem:

interface Cache
{
    public function isNotFresh($lifetime);

    public function put($url);

    public function get();
}

class FileCache implements Cache
{
    private $cacheFilePath;

    public function __construct()
    {
        $this->cacheFilePath = __DIR__ . '/../../cache/random';
    }

    public function isNotFresh($lifetime)
    {
        return !file_exists($this->cacheFilePath)
                || time() - filemtime($this->cacheFilePath) > $lifetime
    }

    public function put($url)
    {
        file_put_contents($this->cacheFilePath, $url);
    }

    public function get()
    {
         return file_get_contents($this->cacheFilePath);
    }
}

class CachedCatApi implements CatApi
{
    ...
    private $cache;

    public function __construct(CatApi $realCatApi, Cache $cache)
    {
        ...
        $this->cache = $cache;
    }

    public function getRandomImage()
    {
        if ($this->cache->isNotFresh()) {
            ...

            $this->cache->put($url);

            return $url;
        }

        return $this->cache->get();
    }
}

Finally, finally, we can remove those ugly sleep() statements from the CachedCatApiTest as well, since can use a simple stand-in for Cache. I leave this part of the exercise to the imagination of the reader.

There are some issues left here:

  1. I don't like the API of Cache. Cache::isNotFresh() is hard to wrap your head around. It also doesn't correspond to existing cache abstractions like the one from Doctrine, which makes it harder to follow for people familiar with caching things in PHP.
  2. The cache file path is still hard-coded in the FileCache class. This is bad for testing since you can't change it for testing purposes.

The first issue can be solved by renaming some methods and inverting some of the boolean logic. The second issue can be solved by simply injecting the path as a constructor argument.

Conclusion

In this part we've hidden a lot of the low-level details about file handling and HTTP requests from sight. This allowed us to turn our tests into proper unit tests.

Of course, the code in FileCache and FileGetContentsHttpClient still needs to be tested. So you will still end up with some slow and fragile tests. There's one very helpful thing you can do: push these tests out of your own project by using existing solutions for doing filesystem manipulations and making HTTP requests. The burden of testing the correctness of that code is upon the shoulders of the maintainers of these libraries. This basically allows you to focus on the important parts of your code and will keep your unit test suite very fast.

Read part III

PHP object design refactoring
Comments
This website uses MailComments: you can send your comments to this post by email. Read more about MailComments, including suggestions for writing your comments (in HTML or Markdown).
Christoph Ueberschaer

Hi Matthias,

great posts!

I have a question concerning the use of vendor libraries:

The Dependency Inversion Principle states that "High-level modules should not depend on low-level modules. Both should depend on abstractions". As far as I understand this means you should define interfaces as needed by your higher-level modules and lower-level modules have to implemented it.

When using a library like guzzle would you recommend to define a specific interface for the needed use case (like the HttpClient interface in your CatAPI) and wrap the needed functionality of the library into a class that implements that interface or would you rather use the simpler solution and just inject the library class as a dependency into your higher-level module (like the constructor function of your RealCatApi class).

In my opinion the second solution would offend the DIP because your higher-level module would be bound to a concrete implementation of a lower-level module. What is your opinion on this?

Xu Ding

HI Matthias

Awesome series.

One question though. How would you exactly move tests of FileCache and FileGetContentsHttpClient out of own project.

Do you mean we can find some libraries providing the same exact interface as Cache and HttpClient.

Thanks in advanced.

Regards,
Xu

Matthias Noback

Yes, sorry, maybe I didn't make this clear. I should have pointed to libraries like Doctrine Cache and Guzzle (for making HTTP requests). They have proper test suites proving that their code works. Just delegate as much of this low-level, sometimes difficult and unpredictable stuff to a vendor library.

Tito Miguel Costa

I think the implementation of the method CachedCatApi::getRandomImage is wrong, the method cacheIsNotFresh does not exist, seems to be $this->cache->isNotFresh(time()) and the methods store and retrieve are not part of the signature of the Cache object, should be get and put respectively. Keep up the good work, looking forward to reading next chapter.

Matthias Noback

Sorry, yeah - fixed this. Thanks!