0
\$\begingroup\$

I have an abstract class Consumer:

abstract class Consumer
{
    const API_DOMAIN = 'https://test-api.com/v1/';

    private $client = null;
    private $credentials = [];

    public function __construct(array $credentials)
    {
        $this->client = new Client(); //This is GuzzleHttp/Client.
        $this->credentials = $credentials; // An array
    }


    abstract protected function getEndpoint();

    abstract protected function getHttpMethod();

    abstract protected function getParameter();

    abstract protected function getHeaders();

    abstract protected function getContent();

    abstract protected function parseResponse($result);


    protected function getUrl()
    {
        //This has to be fixed to accept an array of parameters: [key => value] format
        return implode('/', array_filter([self::API_DOMAIN, $this->getEndpoint(), $this->getParameter()]));
    }

    public function execute()
    {
        if (empty($this->credentials))
            throw new InvalidCredentialsException();
        try {
            $response = $this->client->request($this->getHttpMethod(), $this->getUrl(), [
                'auth' => $this->credentials,
                'form_params' => $this->getContent()
            ]);
            return $this->parseResponse(json_decode($response->getBody()->getContents()));
        } catch (\Exception $e) {
            throw new ErrorResponseException($e->getMessage(), $e->getCode());
        }
    }
}

And if I have an endpoint that needs to be accessed via GET I do the following:

class ListAll extends Consumer
{

    protected function parseResponse($result)
    {
        //Collection class from illuminate/support package
        //by the creators of Laravel.
        $collection = new Collection();
        foreach ($result as $item) {
            $collection->push(new Server($item));
        }
        return $collection;
    }


    public function getEndpoint()
    {
        return 'users';
    }

    public function getParameter()
    {
        return null; //No parameters required.
    }

    public function getHeaders()
    {
        return []; //No specific headers is going to be sent.
    }

    public function getContent()
    {
        return []; //No content is going to be sent.
    }

    public function getHttpMethod()
    {
        return 'GET';
    }
}

And the class above is pointing to an endpoint that requires no parameters, headers or content. However if it's a POST request (let's say the endpoint is for creating a user) I must have a set() method for content atleast. So I made a trait:

trait SetContentTrait
{
    protected $content = [];

    public function setContent($content)
    {
        $this->content = $content;
    }
}

And I'm using it in my Create class:

class Create extends Consumer
{
    use SetContentTrait;

    protected function parseResponse($result)
    {
        return new User($result);
    }

    protected function getEndpoint()
    {
        return 'users';
    }

    protected function getHttpMethod()
    {
        return 'POST';
    }

    public function getParameter()
    {
        return null;
    }

    public function getHeaders()
    {
        return [];
    }

    public function getContent()
    {
        return $this->content;
    }
}

Whenever I need to call that API I do the following:

$create = new Create($credentials);
$user = [
    'username' => 'Iv',
    'password' => 'SOMEHASH',
    'email' => 'my-email@gmai.com'
];

$create->setContent($user);
$user = $create->execute();

So is it a good approach to use traits in this case? Not every endpoint requires parameters, headers and content.

\$\endgroup\$
1
\$\begingroup\$
  • Your class names are a little generic, so it is hard for me to get a feel for your intent here. Create seems like an odd name for a class. Maybe UserFactory, UserProvider or similar. If Create is ultimately intended to be a more generic factory type of class that could create any sort of entity depending on what endpoint is used, then perhaps a name like EntityFactory or similar would be more appropriate. Try to name classes, methods, etc. to clearly indicate their purpose.

  • If you are getting advanced enough in your coding to start considering using traits, then you are advanced enough to write proper Doc blocks. Sorry, not trying to shame here, but this is generally well thought out code compared to many samples on this site. I want to see you take it to the next level in terms of being closer to production-ready code in a professional environment.

  • Consider implementing concrete methods (which could be overridden) in your abstract class. You are repeating yourself a lot in extending classes with implementing your getters.

  • So do you have a second use case where you are going to use that trait? It probably doesn't make sense to make a trait unless you have other use cases where you need to consume it. For this single use case you show here, I don't know why this functionality would not just be part of base class.

  • From a coding style standpoint, you are doing a few thiings that are generally frowned upon. You are putting comments out to the right of code instead of above the code they are applicable to, and you have several lines the go beyond the typical "best practice" of keeping lines of code to an 80 character limit. These make code harder to read.

Some more specific comments follow:

const API_DOMAIN = 'https://test-api.com/v1/';

Why is this a constant on this class? If you want to make this class more flexible, perhaps it is passed on constructor or set via setter. If this is very narrow use class, perhaps at least consider moving this to app configuration.


public function __construct(array $credentials)
{
    $this->client = new Client(); //This is GuzzleHttp/Client.
    $this->credentials = $credentials; // An array
}

Can these credentials be passed in an object that can be better type hinted? You should take appropriate steps to validate this $credentials parameter as a valid dependency (and throw Exception if it is not) before working with at and setting it on the object. Perhaps throw InvalidArgumentException upon validation failure.

Consider passing the Guzzle client to constructor as dependency (dependency injection). Why should this class need to know how to instantiate its own dependencies and the potential error/exception handling around them (which is absent here) in order to work with the dependency? It should just get a valid dependency this is properly set up for it to begin working with.

Get in good habit of namespacing. Wherever you instantiate the guzzle client you should do it like new \GuzzleHttp\Client(). This is much clearer (eliminates the need for your comment). Similarly if passing as a dependency, you should type hint as \GuzzleHttp\Client $guzzle_client.


protected function getUrl()
{
    //This has to be fixed to accept an array of parameters: [key => value] format
    return implode('/', array_filter([self::API_DOMAIN, $this->getEndpoint(), $this->getParameter()]));
}

Do you really need to go through this exercise every time or should Url just be set on object?


if (empty($this->credentials))
    throw new InvalidCredentialsException();

As mentioned previously, this should have been validated in the constructor and failed long before the point where a client would ever call this method.


         return $this->parseResponse(json_decode($response->getBody()->getContents()));

Odd to see a return within a try block. Does this code even potentially throw? There really is too much going on in a single line of code and REALLY assumes a happy path operation of all 4 underlying operations. There might be need to better handling of edge cases here (what if JSON doesn't decode? what if "parsing" doesn't work?).


    } catch (\Exception $e) {
        throw new ErrorResponseException($e->getMessage(), $e->getCode());
    }

This is kind of a meaningless try-catch block. It takes a more generic exception type and turns it into a more specific exception (an odd pattern in and of itself), but doesn't really do anything meaningful here like log the failure or add additional context to the underlying exception to the caller. How is this any better than the caller just getting the underlying Exception directly?


    $collection = new Collection();
    foreach ($result as $item) {
        $collection->push(new Server($item));

Namespaces?


protected function getEndpoint()
{
    return 'users';
}

protected function getHttpMethod()
{
    return 'POST';
}

Endpoint, HttpMethod, headers, content, etc. are acting here as de facto class constants or properties when you do something like return 'some hard coded value';.

These should be actual class properties or constants. This probably also removes the need to override these methods in every implementing class (if you put concrete methods in base class) as well as need to define some subset of these as traits.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for your answer! I will look through all of your comments in details. Doc blocks are not added as the question was about the approach of using traits for this kind of problem. I appreciate your time and again thanks a lot! \$\endgroup\$ – FakeHeal Oct 3 '16 at 7:25
  • 1
    \$\begingroup\$ @FakeHeal Glad to provide feedback. On any future code reviews, I would encourage you to leave your doc blocks in. This helps the reviewer and gives good opportunity for the code writer to provide context into how code is intended to work. \$\endgroup\$ – Mike Brant Oct 3 '16 at 17:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.