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

PHP wrapper around an API - best practices

Submitted by: @import:stackexchange-codereview··
0
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

``
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:

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.