Bonus - Refactoring Part 1

After watching through this series, I had a query from a site member regarding a better way to handle the dependency on Guzzle.

Here is the question:

In that tutorial you mentioned that 'new'ing' the Guzzle client was bad practice, I assume that's because it's repeated code that you have to copy and paste into each function in the service. So, after that I was looking for a tutorial describing how to create it through dependency injection (which is what I think you said the correct way was)

It's a good point, and one that I hadn't actually addressed in the series.

The 'copy / paste' argument is really just a small part of the problem.

It's really painful to test classes where calls to new are made in the constructor. It's considered a really bad practice. Don't do this.

I'd also really like to only define a service once and keep re-using it, rather than instantiating new instances of it all over the shop.

So, why on Earth did I do it like this for the video?

Well, in truth, if I were to use Guzzle inside a Symfony project - unless I had a very good reason not to do so - I would rely on a third party bundle, such as Eightpoints Guzzle Bundle.

It's only when digging into 'a better way' of using Guzzle over simply new'ing up that you will run into a (likely) unanticipated problem. You can't inject Guzzle's Client without having new'ed it up somewhere to begin with.

Doing so requires a service definition, but it initially seems like chicken and the egg - we know not to do it this way, but there doesn't seem to be any other way. Well, we will cover how third party bundles do this in the next video.

But for now, we will start by refactoring our code to isolate this problem, instead relying on an interface we will define and use to hide the untestable mess laying behind it.

Once we have the interface in place, in the next video we will use a third party bundle to remove this problem altogether.

Defining an Interface to Decouple our Problematic Code

At this scale of project, what I am about to discuss is really not that big of a concern.

But it's pretty poor practice, and even with such a small project the rot has already begun to set in. We can do better.

Currently we have implementation details spilling into our code:

<?php

// src/AppBundle/Service/GitHubApi.php

namespace AppBundle\Service;

use GuzzleHttp\Client;

class GitHubApi
{
    public function getProfile($username)
    {
        $client = new Client();
        /** @var $response \Psr\Http\Message\ResponseInterface */
        $response = $client->request('GET', 'https://api.github.com/users/' . $username);

        $data = json_decode($response->getBody()->getContents(), true);

Our GitHubApi service is directly tied to Guzzle.

Switching out from Guzzle - maybe to Buzz - would involve changing all three lines here. Of course, we duplicated this code into the other function, so that's six lines.

Also, our class is called GitHubApi, so if you were completely new to this project, you would likely be surprised to find all this code about Guzzle... aren't we interested in GitHub? What is Guzzle? Questions, questions.

Thinking about it, GitHubApi is probably a bad name for this service. We are communicating with GitHub's API, but the class itself is more of a data provider. Anyway, for the moment we will stick with the name of GitHubApi, but it's something to be thinking about as we begin the refactoring.

The fact is, how we get the data from GitHub is not the concern of this class. The responsiblity of this class is to provide a user's GitHub profile and repository data in a structured way. That's about it.

Of course, that data has to arrive from somewhere. And so in the scale and scope of this application, it made sense that inside that this class should ask GitHub for the data, and then return it in a single method call. Not amazing code, and there's no error handling either, but it's good enough at this stage.

Given that we need to ask GitHub for some information, we can deduce that we likely need a get method, much like the HTTP GET verb which we will be using to really retrieve the data. Let's start with this on our interface:

<?php

// /src/AppBundle/Service/HttpClientInterface.php

namespace AppBundle\Service;

interface HttpClientInterface
{
    public function get($url);
    public function post($url, $data);
}

I've added in the post method as we may wish to talk back to GitHub along the way, but for the purposes of this tutorial we won't be actually implementing that method. It's just for illustration.

Our interface is really simple.

We want our GitHubApi service to make calls to get($someUrl), and let a more specific HTTP client implementation worry about how that really happens.

Knowing this, we can go ahead and create a new class - GuzzleHttpClient - which will take those three lines of code from our existing getProfile method, and extract them into the get method of the new class:

<?php

// /src/AppBundle/Service/GuzzleHttpClient.php

namespace AppBundle\Service;

class GuzzleHttpClient implements HttpClientInterface
{
    public function get($url)
    {
        $client = new \GuzzleHttp\Client();

        $response = $client->get($url);

        return json_decode($response->getBody()->getContents(), true);
    }

    public function post($url, $data)
    {
        // not in use, but we must define this method
        // as it is part of the HttpClientInterface interface
    }
}

We'd likely want to share the client between the get and post methods, so we might think about moving the instantiation of \GuzzleHttp\Client to the constructor:

<?php

// /src/AppBundle/Service/GuzzleHttpClient.php

namespace AppBundle\Service;

class GuzzleHttpClient implements HttpClientInterface
{
    private $client;

    public function __construct()
    {
        // don't do this
        $this->client = new \GuzzleHttp\Client();
    }

    public function get($url)
    {
        $response = $this->client->get($url);

        return json_decode($response->getBody()->getContents(), true);
    }

    // * snip *

Again this is really bad practice!.

We will cover a better way to do this shortly. All we have done so far is to move the problem.

Injecting Our GuzzleHttpClient Into GitHubApi Service

Now that we have the standalone GuzzleHttpClient we can add an entry in Symfony's services.yml to expose this class as a service:

# /app/config/services.yml

services:
    guzzle_http_client:
        class: AppBundle\Service\GuzzleHttpClient

    github_api:
        class: AppBundle\Service\GitHubApi
        arguments:
            - "@guzzle_http_client"

We need to update the constructor in GitHubApi letting it know that it will receive something that implements HttpClientInterface:

<?php

// /src/AppBundle/Service/GitHubApi.php

namespace AppBundle\Service;

class GitHubApi
{
    /**
     * @var HttpClientInterface
     */
    private $httpClient;

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

    // * snip * 

And as soon as we do this, we can change both the getProfile and getRepos methods:

// /src/AppBundle/Service/GitHubApi.php

    public function getProfile($username)
    {
        $data = $this->httpClient->get('https://api.github.com/users/' . $username);

        // * snip *
    }

    public function getRepos($username)
    {
        $data = $this->httpClient->get('https://api.github.com/users/' . $username . '/repos');

        // * snip *
    }

And at this stage, our site should still work just fine.

Remember, all we have done at this stage is to move the problem. We haven't fixed it.

Still, we have isolated the rotten code, which should make our lives that little bit easier.

Giving Ourselves A Buzz

What's really cool now is that we could very easily swap out the Guzzle implementation for another - Buzz being the one that immediately jumped to my mind.

All we need to do is to implement the same interface and make sure we return data in the same format, and we are good to go.

Note: The implementation for Buzz changed significantly in 0.17.x. Please use composer require kriswallsmith/buzz:0.16.1 for compatibility with the video content.

As we are returning an array from our getProfile and getRepos methods, we would definitely want some tests around this as it's easy to make a mistake with array keys and such-like. A better way would be to use objects, but again, let's keep things as simple as possible for the moment.

<?php

// /src/AppBundle/Service/BuzzHttpClient.php

namespace AppBundle\Service;

use Buzz\Message\Request;
use Buzz\Message\Response;

class BuzzHttpClient implements HttpClientInterface
{
    private $client;

    public function __construct()
    {
        // bad practice, please don't do this
        $this->client = new \Buzz\Client\Curl();
        $this->client->setOption(CURLOPT_USERAGENT, 'Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2228.0 Safari/537.36');
    }

    public function get($url)
    {
        $request = new Request('GET', $url);
        $response = new Response();

        $this->client->send($request, $response);

        return json_decode($response->getContent(), true);
    }

    public function post($url, $data)
    {
        // not in use, but we must define this method
        // as it is part of the HttpClientInterface interface
    }
}

Even though the implementation looks very different to the Guzzle Client, we can use this with our GitHubApi without any changes. All we need to do is tell Symfony about this new service, and change the service we are injecting into github_api:

# /app/config/services.yml

services:
    guzzle_http_client:
        class: AppBundle\Service\GuzzleHttpClient

    buzz_http_client:
        class: AppBundle\Service\BuzzHttpClient

    github_api:
        class: AppBundle\Service\GitHubApi
        arguments:
            - "@buzz_http_client"

And it should all just work fine.

Again, we've new'ed up in the constructor which really bad practice.

In the next video we will cover a better way to do this.