patternphpMinor
Curl-based REST client library (round 2)
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
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
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
-
The methods corresponding to the HTTP verbs (I.e.
-
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
-
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
Perhaps if more regular expressions features (e.g. anchors like
And the loop here could likely be simplified:
In that loop there is no
Instead of having an array of patterns, use a pattern like
-
I know this is subjective so if you don't agree then ignore it: for setter methods of boolean properties like
-
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.