Pussies: Refactoring

Good afternoon, Habrovchane!



I continue to combine the development of translation skills with English and the study of interesting, from my point of view, programming materials and share with you a slightly adapted translation of the first part of a short series of articles on refactoring from the Dutchman Matthias Noback, who lives in Zeist, near Utrecht.



For the most part, in three articles we are talking about refactoring, namely, the selection of individual entities and the creation of independent parts of the code, which are then convenient to test and modify. This, of course, will lead to an increase in the number of abstractions and complication of the whole task, but Noback cannot do without it.

')

The meaning of this article is not affected, and all changes (title and a couple of substitutions for the name of the service) were made only to dilute the content dry article and, I hope, simple article in terms of material complexity, to then go on to the next parts of the whole cycle.



So, for starters, here's a piece of code for you:

class CatApi { public function getRandomImage() { if (!file_exists(__DIR__ . '/../../cache/random') || time() - filemtime(__DIR__ . '/../../cache/random') > 3) { $responseXml = @file_get_contents( 'http://thecatapi.com/api/images/get?format=xml&type=jpg' ); if (!$responseXml) { //     -    return 'http://cdn.my-cool-website.com/default.jpg'; } $responseElement = new \SimpleXMLElement($responseXml); file_put_contents( __DIR__ . '/../../cache/random', (string)$responseElement->data->images[0]->image->url ); return (string)$responseElement->data->images[0]->image->url; } else { return file_get_contents(__DIR__ . '/../../cache/random'); } } } 

As you can see, the getRandomImage () function returns the random address of a picture from api pussies ( yes, it exists! ). If the function is called several times within 3 seconds, the same result is returned as before, in order to prevent a heavy load on the API and, accordingly, slow responses.



If this code worked according to the “once launched and received the answer” principle, it would not be so important what it is. But since all this will end with a real application that works in production, you will have to refactor.



The problems that I see here:

1. One function solves two tasks — getting a random address and caching it.
2. There are low-level details in the code, such as paths to files, addresses, cache life cycle, XML names — all of which are hard-coded, which in turn affects the understanding of what actually happens.

All together, this in itself is a problem - let's take a look, at least, at the “unit test” for all this:

 class CatApiTest extends \PHPUnit_Framework_TestCase { protected function tearDown() { @unlink(__DIR__ . '/../../cache/random'); } /** @test */ public function it_fetches_a_random_url_of_a_cat_gif() { $catApi = new CatApi(); $url = $catApi->getRandomImage(); $this->assertTrue(filter_var($url, FILTER_VALIDATE_URL) !== false); } /** @test */ public function it_caches_a_random_cat_gif_url_for_3_seconds() { $catApi = new CatApi(); $firstUrl = $catApi->getRandomImage(); sleep(2); $secondUrl = $catApi->getRandomImage(); sleep(2); $thirdUrl = $catApi->getRandomImage(); $this->assertSame($firstUrl, $secondUrl); $this->assertNotSame($secondUrl, $thirdUrl); } } 

What is bad? To test the cache, we need a couple of sleep calls. Also, we check that the return value is a valid address (there is no verification that the value is the address of the pussies).



Let's try to improve the situation a little

We separate approaches - caching and real requests

When you look at the code above, it becomes clear that caching is intertwined with getting a new image. But this is not even as bad as it may seem. After all, usually the code for such a script is something like this:

 if (!isCachedRandomImageFresh()) { $url = fetchFreshRandomImage(); putRandomImageInCache($url); return $url; } return cachedRandomImage(); 

It makes no difference whether something is being loaded in “lazy” mode (lazy-load) or it is again generated every X seconds - this pattern will be present in one form or another in the code.



Dividing these two things into different parts, we will have the opportunity to test the receipt of a new image separately from its caching. It makes no sense to know about the details of caching, when the main task is to check that the code really gets different pictures.



First, we will render all the code related to caching into a new class with the same interface:

 class CachedCatApi { public function getRandomImage() { $cacheFilePath = __DIR__ . '/../../cache/random'; if (!file_exists($cacheFilePath) || time() - filemtime($cacheFilePath) > 3) { //    -    $realCatApi = new CatApi(); $url = $realCatApi->getRandomImage(); file_put_contents($cacheFilePath, $url); return $url; } return file_get_contents($cacheFilePath); } } 

Plus, I changed some mistakes and now the code reads better:



1. Removed all unnecessary references __DIR__. '/../../cache/random'

2. Removed the block with else - it is not needed at all, as if always returns something.

3. The CatApi class now contains only API request logic:

 class CatApi { public function getRandomImage() { $responseXml = @file_get_contents('http://thecatapi.com/api/images/get?format=xml&type=jpg'); if (!$responseXml) { //     -    return 'http://cdn.my-cool-website.com/default.jpg'; } $responseElement = new \SimpleXMLElement($responseXml); return (string)$responseElement->data->images[0]->image->url; } } 

Inversion of dependencies: the introduction of abstractions

We can already divide the test now, because we have two separate classes, but there is one thing: we will have to test the CatApi class twice, because CachedCatApi still uses it directly. You can make a big step towards improvement - introduce an abstraction for CatApi to replace it when we don’t have the desire to make real HTTP requests while testing the caching behavior of CachedCatApi.



To do this, we will single out a separate interface for two classes, since both of these classes use the same public interface ( meaning that two different classes at the moment have the same public method getRandomImage () ). Let's call it CatApi, and rename the old class to RealCatApi.

 interface CatApi { /** * @return string */ public function getRandomImage(); } class RealCatApi implements CatApi { ... } class CachedCatApi implements CatApi { ... } 

Now you need to prevent CachedCatApi from creating CatApi objects. Instead, we will be able to use it to get a new, random pussy. This approach is called dependency injection: we provide an object with the necessary requirements.

 class CachedCatApi implements CatApi { private $realCatApi; public function __construct(CatApi $realCatApi) { $this->realCatApi = $realCatApi; } public function getRandomImage() { ... $url = $this->realCatApi->getRandomImage(); ... } } 

Now we can really split the unit tests into two separate ones:

 class CachedCatApiTest extends \PHPUnit_Framework_TestCase { protected function tearDown() { @unlink(__DIR__ . '/../../cache/random'); } /** @test */ public function it_caches_a_random_cat_gif_url_for_3_seconds() { $realCatApi = $this->getMock('RealCatApi'); $realCatApi ->expects($this->any()) ->will($this->returnValue( //       'http://cat-api/random-image/' . uniqid() ); $cachedCatApi = new CachedCatApi($realCatApi); $firstUrl = $cachedCatApi->getRandomImage(); sleep(2); $secondUrl = $cachedCatApi->getRandomImage(); sleep(2); $thirdUrl = $cachedCatApi->getRandomImage(); $this->assertSame($firstUrl, $secondUrl); $this->assertNotSame($secondUrl, $thirdUrl); } } class RealCatApiTest extends \PHPUnit_Framework_TestCase { /** @test */ public function it_fetches_a_random_url_of_a_cat_gif() { $catApi = new RealCatApi(); $url = $catApi->getRandomImage(); $this->assertTrue(filter_var($url, FILTER_VALIDATE_URL) !== false); } } 

Conclusion

What have we achieved? Now we can test caching separately from the usual behavior - getting a random image from the API. This means that these two scenarios can be developed separately from each other, maybe even in completely different directions, while preserving the overall performance, as they adhere to the same contract (interface).



What is left to do? In general, a lot of things. It is still not possible to test RealCatApi bypassing the creation of real HTTP requests (which reduces the reliability of such a test and spoils the very concept of such a test as a “unit test”). The situation is similar with CachedCatApi - it tries to cache into the file system, which again, we don’t want to do in the unit test, as this is slow and affects the global state.



In the second part, you will learn how to get rid of file system dependencies and perform real HTTP requests. Well, then Matthias Noback will play around with URL and XML.



UPD : Pussies: Refactoring. Part two or addiction treatment