patternphpMinor
PHP wrapper around an API - best practices
Viewed 0 times
aroundphpwrapperpracticesapibest
Problem
Here's a PHP wrapper around a public OAuth2 REST API. I would love to hear your thoughts and comments. Testing was a bit frustrating since I tried for my first time. Not sure if I have done the mocking properly.
You can see the whole project on Github
``
* @param string $client_id Client's ID provided by Feedly's Administrators
* @param string $client_secret Client's S
You can see the whole project on Github
``
class Feedly {
private
$_apiBaseUrl = "https://cloud.feedly.com",
$_authorizePath = "/v3/auth/auth",
$_accessTokenPath = "/v3/auth/token",
$_storeAccessTokenToSession;
/**
* @param boolean $sandbox Enable/Disable Sandbox Mode
* @param boolean $storeAccessTokenToSession Choose whether to store the Access token
* to $_SESSION or not
*/
public function __construct($sandbox=FALSE, $storeAccessTokenToSession=TRUE) {
$this->_storeAccessTokenToSession = $storeAccessTokenToSession;
if($this->_storeAccessTokenToSession) session_start();
if($sandbox) $this->_apiBaseUrl = "https://sandbox.feedly.com";
}
/**
* Return authorization URL
* @param string $client_id Client's ID provided by Feedly's Administrators
* @param string $redirect_uri Endpoint to reroute with the results
* @param string $response_type
* @param string $scope
*
* @return string Authorization URL
*/
public function getLoginUrl ($client_id, $redirect_uri,
$response_type="code", $scope="https://cloud.feedly.com/subscriptions") {
return($this->_apiBaseUrl . $this->_authorizePath . "?" .
http_build_query(array(
"client_id"=>$client_id,
"redirect_uri"=>$redirect_uri,
"response_type"=>$response_type,
"scope"=>$scope
)
)
);
}
/**
* Exchange a code got from getLoginUrl for an Access Token`* @param string $client_id Client's ID provided by Feedly's Administrators
* @param string $client_secret Client's S
Solution
Two minor notes:
-
The following is not too easy to read:
Readers have to be really familar with curl parameters or have to check the documentation. It says the following:
The contents of the "Accept-Encoding: " header. This enables decoding
of the response. Supported encodings are "identity", "deflate", and "gzip".
If an empty string, "", is set, a header containing all supported encoding
types is sent.
But it's
where
-
Comments like this is unnecessary:
The function name says the same, so I'd remove the comment. In the following case I'd rename the function to
(Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)
-
The following is not too easy to read:
curl_setopt($r, CURLOPT_ENCODING, 1);Readers have to be really familar with curl parameters or have to check the documentation. It says the following:
The contents of the "Accept-Encoding: " header. This enables decoding
of the response. Supported encodings are "identity", "deflate", and "gzip".
If an empty string, "", is set, a header containing all supported encoding
types is sent.
But it's
1, which isn't a string. 1 == ""? I suggest the following:curl_setopt($r, CURLOPT_ENCODING, ALL_SUPPORTED_ENCODING_TYPES);where
ALL_SUPPORTED_ENCODING_TYPES is a constant with an empty string value. (I suppose 1 == "".)-
Comments like this is unnecessary:
/**
* Testing GetAccessToken
*/
public function testGetAccessToken()The function name says the same, so I'd remove the comment. In the following case I'd rename the function to
testValidReturnedUrlForAuthorization() and remove the comment:/**
* Test valid returned URL for authorization
*/
public function testGetLoginURL()(Clean Code by Robert C. Martin: Chapter 4: Comments, Noise Comments)
Code Snippets
curl_setopt($r, CURLOPT_ENCODING, 1);curl_setopt($r, CURLOPT_ENCODING, ALL_SUPPORTED_ENCODING_TYPES);/**
* Testing GetAccessToken
*/
public function testGetAccessToken()/**
* Test valid returned URL for authorization
*/
public function testGetLoginURL()Context
StackExchange Code Review Q#40767, answer score: 4
Revisions (0)
No revisions yet.