HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Curl-based REST client library (round 2)

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
restclientlibrarybasedroundcurl

Problem

This is a second iteration on an earlier review - cURL based REST client library

I have done some refactoring to split out the REST HTTP response into it's own class from what was in the previous review and implement.

You might find it useful to read the library's README file before performing this review for further background and usage examples, which I have omitted here for brevity and to allow the question to focus on the code.

I am only reviewing the single REST call portion of the library here.

I have also opened a separate review to cover the curl_multi_* based functionality for performing multiple requests in parallel.

RestClient class

```
validateAction($action);
$this->curlSetup();
$this->setRequestUrl($action);
curl_setopt($this->curl, CURLOPT_HTTPGET, true);
// execute call. Can throw \Exception.
$this->curlExec();

return $this->response;
}

/**
* Method to exexute POST on server
*
* @param mixed $action
* @param mixed $data
* @return CurlHttpResponse
* @throws \InvalidArgumentException
* @throws \Exception
*/
public function post($action, $data) {
$this->validateAction($action);
$this->validateData($data);
$this->curlSetup();
$this->setRequestUrl($action);
$this->setRequestData($data);
curl_setopt($this->curl, CURLOPT_POST, true);
// execute call. Can throw \Exception.
$this->curlExec();

return $this->response;
}

/**
* Method to execute PUT on server
*
* @param string $action
* @param mixed $data
* @return CurlHttpResponse
* @throws \InvalidArgumentException
* @throws \Exception
*/
public function put($action, $data) {
$this->validateAction($action);
$this->validateData($data);
$this->curlSetup();
$this->setRequestUrl($action);
$this->setRequestData($data);
curl_setopt($thi

Solution

It appears that much of the advice from Greg's answer and Peter's answer to your first post have been incorporated. I like the extensive use of docblocks throughout the code. There are a few suggestions I have:

-
Did you consider implementing the PSR-7 interfaces such as Psr\Http\Message\ServerRequestInterface, Psr\Http\Message\UriInterface, etc.?

-
The methods corresponding to the HTTP verbs (I.e. get, post) seem a bit repetitive. The common functionality could be abstracted into the curlExec() or curlSetup method or else a new setup method - to validate the action and data, set headers, conditionally call setRequestData(), etc.

-
Newer features of PHP 7-8+ offer means to simplify the code - e.g. obviously array syntax is one, but also declaring types for parameters and return values could help - potentially eliminating the need for methods like validateAction(), manual checks on arguments - e.g. which throw InvalidArgumentExceptions, etc.

-
While the performance tag wasn't added, it would be wise to consider optimizations - including speed of function calls. Namely - using string functions instead of regular expressions. Instead of using preg_match() some places strpos() could be used and would be faster - e.g.

$httpsPattern = '#https://#i';
 $httpPattern = '#http://#i';
 if (1 === preg_match($httpsPattern, $host)) {


Perhaps if more regular expressions features (e.g. anchors like ^) were used it would be different but this example merely checks to see if a string is within another string.

And the loop here could likely be simplified:

$portPatterns = array(
    '/:443$/',
    '/:8443$/',
);
foreach ($portPatterns as $pattern) {
    if (1 === preg_match($pattern, $host)) {
        $this->setUseSsl(true);
    }
}


In that loop there is no break statement so if the first loop called the method, it would still continue to loop.

Instead of having an array of patterns, use a pattern like /:8?443$/ or /:(8443|443)$/ to eliminate the loop.

-
I know this is subjective so if you don't agree then ignore it: for setter methods of boolean properties like setUseSsl() consider adding a default value - e.g. true, given the default value of $useSsl is false, so when it gets called it is typically to switch the value.

Code Snippets

$httpsPattern = '#https://#i';
 $httpPattern = '#http://#i';
 if (1 === preg_match($httpsPattern, $host)) {
$portPatterns = array(
    '/:443$/',
    '/:8443$/',
);
foreach ($portPatterns as $pattern) {
    if (1 === preg_match($pattern, $host)) {
        $this->setUseSsl(true);
    }
}

Context

StackExchange Code Review Q#157165, answer score: 3

Revisions (0)

No revisions yet.