1
\$\begingroup\$

The project is an API that will itself make HTTP requests to external webservices.

Since this is PHP, we actually chose to use guzzlehttp. Now the library itself doesn't matter much, but I'm scared we'll be stuck with it if it ever has a major change (it already happened in the past).

So I thought I'd create an interface that would be common to be able to swap the implementation if needed. Here it is (I removed the Use statements for clarity) :

<?php

namespace App\Core\Request;

interface HttpRequest
{
    public function get();
    public function post();
    public function setUri($uri);
    public function setDatas(array $datas);
    public function setHeaders(array $headers);
}

I thought this would be usable by cURL if we need it.

Then I made the Guzzle implementation, here it is :

<?php

namespace App\Core\Request;

class GuzzleHttpRequest implements HttpRequest
{
    protected $client;
    protected $logger;
    protected $datas   = [];
    protected $headers = [];
    protected $uri;
    protected $requestLog;
    protected $currentRequestLog;
    protected $serviceId;

    const DEFAULT_ERROR_CODE = 500;

    public function __construct(Client $client, GuzzleLogger $logger, RequestsLog $requestLog)
    {
        $this->client     = $client;
        $this->logger     = $logger;
        $this->requestLog = $requestLog;
        $this->serviceId  = Config::get('app.locale');
    }

    public function setDatas(array $datas)
    {
        $this->datas = $datas;
    }

    public function setHeaders(array $headers)
    {
        $this->headers = $headers;
    }

    public function get()
    {
        return $this->request('GET', 'query');
    }

    public function post()
    {
        return $this->request('POST', 'json');
    }

    protected function request($method, $dataType)
    {
        try {
            $response = $this->sendRequest($method, $dataType);

            return $this->handleResponse($response);
        }
        catch (RequestException $e) {
            return $this->handleRequestException($e);
        }
        catch (Exception $e) {
            return $this->handleException($e);
        }
    }

    protected function sendRequest($method, $dataType)
    {
        $this->checkUri();

        $this->currentRequestLog = $this->requestLog->fromConnection($this->serviceId, $this->getUri());

        return $this->client->request($method, $this->getUri(), $this->getOptions($dataType));
    }

    protected function checkUri()
    {
        if (is_null($this->getUri())) {
            throw new UriNotSetException('Uri should be set.', self::DEFAULT_ERROR_CODE);
        }
    }

    protected function getUri()
    {
        return $this->uri;
    }

    public function setUri($uri)
    {
        $this->uri = $uri;
    }

    protected function getOptions($dataType)
    {
        return [$dataType => $this->datas, 'headers' => $this->headers, 'handler' => $this->getHandler('generic')];
    }

    protected function getHandler($logName)
    {
        $uniqId = uniqid('REQUEST');

        return $this->logger->createLoggingHandlerStack([
            "REQUEST_LOG_ID_{$this->currentRequestLog->id} REQUEST : \r\n {request}",
            "REQUEST_LOG_ID_{$this->currentRequestLog->id} RESPONSE : {response}",
        ], $logName);
    }

    protected function handleResponse(GuzzleResponse $response)
    {
        $this->currentRequestLog->succeed();

        return ResponseJsonSuccess::getResponse($this->getFormattedResponse($response));
    }

    protected function getFormattedResponse(GuzzleResponse $response)
    {
        return (string)$response->getBody();
    }

    protected function handleRequestException(RequestException $e)
    {
        $this->currentRequestLog->fail();

        $code    = $e->getCode() ?: self::DEFAULT_ERROR_CODE;
        $message = (string)$e->getMessage();

        return $this->handleException(new Exception($message, $code));
    }

    protected function handleException(Exception $e)
    {
        return ResponseJsonError::getResponse($e);
    }
}

This class could be problematic to me as it's doing lots of things. It's able to handle requests and responses (and exceptions bound to these), and it also logs all of its requests.

I did this class to be able to implement subclasses to change the behavior when needed, but I still wanted to be able to use this main class for most requests.

Here's an example of a subclass I made :

<?php

namespace App\Services\Pivotal\Request;

class PivotalGuzzleHttpRequest extends GuzzleHttpRequest
{
    private $tokenFactory;

    public function __construct(Client $client, GuzzleLogger $logger, RequestsLog $requestLog, TokenFactory $tokenFactory)
    {
        parent::__construct($client, $logger, $requestLog);
        $this->tokenFactory = $tokenFactory;
    }

    protected function getFormattedResponse(Response $response)
    {
        $this->currentRequestLog->succeed();

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

    protected function getOptions($dataType)
    {
        $options            = parent::getOptions($dataType);
        $options['headers'] = [
            "Content-Type"  => "application/json",
            "Authorization" => "Bearer {$this->tokenFactory->getTokenId()}"
        ];
        $options['handler'] = $this->getHandler('pivotal');

        return $options;
    }
}

How can I improve this code ? All I'm looking for is the code to be easy to use, easy to test, and easy to understand.

\$\endgroup\$
8
  • \$\begingroup\$ Why remove use statement for clarity when they typically add clarity? \$\endgroup\$
    – Mike Brant
    Commented Oct 26, 2016 at 13:44
  • \$\begingroup\$ I only removed them to paste the code here, as I thought they'd be noise only. \$\endgroup\$ Commented Oct 26, 2016 at 13:46
  • 1
    \$\begingroup\$ Extending the implementation is pointless and defeats the purpose of having the interface. Also looks ugly. I would rather make the httpclient a property of the PivotalRequest class. Since the PivotalRequest class implements actual business logic and the other stuff is just utility code. \$\endgroup\$ Commented Oct 26, 2016 at 13:54
  • 1
    \$\begingroup\$ You create another Interface for that, something like FunctionalRequest or whatever. That one has these methods of manipulating stuff and use the HttpRequest methods. This way you separate the Business Logic from the utility interfaces. \$\endgroup\$ Commented Oct 26, 2016 at 14:06
  • 1
    \$\begingroup\$ So, you have the HttpRequest. It is just an utility for providing a way to do a generic request. Then you have an Interface (or maybe an Abstract Class) and the implementation has an HttpRequest derived object as property. Inside methods like getFormattedResponse, instead of doing this->currentRequestLog->.... you do this->httpRequestObject->currentRequestLog ... Of course, they will need to be public but in regards to the new design, why wouldn't they be public? Since the use of that Interface is to provide that ability, they should be public. The httpObject property will be protected. \$\endgroup\$ Commented Oct 26, 2016 at 14:18

1 Answer 1

2
\$\begingroup\$

Generally, I think this code is well thought out and well structured.

  • Good use of dependency injection, especially with preference for passing type-hintable objects as parameters instead of primitives.
  • Methods seem to have good scope, with specific purposes.

Some general thoughts

  • Consider adding Doc Blocks. This code is well-written, but you could take it to the next level in terms of professionalism by adding these, IMO.
  • A number of lines appear to be flirting with ideal 80 character per line limit. I think there are a number of cases where the code might read better if you broke some of your longer lines up.
  • Though I think you generally do a good job of writing code that is self-explanatory, there are perhaps some points where minimal comments could add value.
  • I agree with sentiment on this class doing too much, a lot of it not specific to usage of the Guzzle client. I actually wonder whether you might consider an abstract class in place of (or perhaps on conjunction with) the use of the interface. This might allow you to move common exception handling, logging, property getting/setting, etc. out of the Guzzle-specific implementation, while still forcing implementation of any abstract methods within inheriting classes.
  • I am not sure about some of your reasoning for then extending Guzzle class class to the Pivotal class. Is this composable enough? Do you think there would be a PivotSomeOtherClientHttpRequest at some point? In other words, is some of the functionality to Pivotal something that can be encapsulated into it's own class which is passed to (or gets passed) an HttpRequest object to perform these operations? Without other examples of how the Guzzle class would potentially be extended, it is hard to make more substantive suggestions.
  • Can data that is going to be set to $this->datas only be an array? This seems restrictive.
  • It is not clear to me how you set a query string for GET request. Do you have to rely on passing this in URI?
  • Consider grouping your getters/setters together rather than splitting them in class with other methods in between.
  • setUri() should have some validation attached to it (i.e. string is passed at minimum, perhaps non-zero-length string if you don't expect the value to be empty string).
  • Consider validation on other setters if, for example, empty arrays are not expected (you are already type-hinting for array type).

Code-specific notes

$this->serviceId  = Config::get('app.locale');

Is Config::get() guaranteed to have a value at this key? Do you need to do any validation that this configuration value is set properly before completing the constructor (vs. throwing exception)? Also variable name and config name do not seem to match. Is the variable being set here a service ID or a locale setting?


public function post()
{
    return $this->request('POST', 'json');
}

Should this method perhaps be called postJson?


protected function getOptions($dataType)
{
    return [$dataType => $this->datas, 'headers' => $this->headers, 'handler' => $this->getHandler('generic')];
}

Not sure I like method name here. getRequestOptions() perhaps? You seem to end up with weird key/value pair here: $dataType => $this->datas. Maybe this is based on underlying Guzzle client but seems like it would make more sense to have two key-value pairs here like 'dataType' => $dataType, 'data' => $this->datas.

This is also a good example of a line that could be broken up like:

    return [
        $dataType => $this->datas,
        'headers' => $this->headers,
        'handler' => $this->getHandler('generic')
    ];
\$\endgroup\$
5
  • \$\begingroup\$ Thanks for your answer. I didn't add Doc Blocks because I didn't see a case where they weren't noise but I think you're right and I found some cases where I could write some. I love the idea of an abstract class since I can already see myself copy pasting setters and getters to a CurlHttpRequest class. Pivotal is actually a webservice, so the implementation is all about this, it's not a "pivot". That means there would be XGuzzleHttpRequest, YGuzzleHttpRequest etc. Right about array $data, I'll pass some DTOs to it too. $dataType is used as either 'query' or 'json' to send the datas ! \$\endgroup\$ Commented Oct 26, 2016 at 14:56
  • \$\begingroup\$ @SteveChamaillard I would question the *GuzzleHTTPRequest, *CurlHttpRequest approach. You are going to end up with a lot of duplicate code to implement endpoint-specific logic for every combination of request client you may want to use. This is why I was suggesting decoupling logic like what what you have in Pivotal... from the Guzzle client logic. This would make your endpoints more composable and interchangeable with actual clients which implement the request. \$\endgroup\$
    – Mike Brant
    Commented Oct 26, 2016 at 16:09
  • \$\begingroup\$ Yes absolutely, someone told me this too. I should have only GuzzleHTTPRequest and CurlHttpRequest and endpoints using these with dependency inversion so that I inject HttpRequest and while bootstrapping I can inject one of the implementation. Is that correct ? \$\endgroup\$ Commented Oct 26, 2016 at 16:19
  • \$\begingroup\$ @SteveChamaillard If you were to take up the suggested refactoring of endpoint config/logic away from HttpClient family of classes, you would obviously need to consider whether things like URI are actually a property on client vs. on endpoint. \$\endgroup\$
    – Mike Brant
    Commented Oct 26, 2016 at 16:19
  • \$\begingroup\$ @SteveChamaillard On the surface, it would seem like perhaps the client should be injected as dependency to an API-specific class which leverages it, but hard to say that definitively without more context around the application. \$\endgroup\$
    – Mike Brant
    Commented Oct 26, 2016 at 16:21

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.