# API call to a music provider

I have inherited a PHP base class for an API call to a music provider. It is functional, however it makes me feel that there are some "magic" nature behind it that makes me un-easy (or maybe I'm not knowledgeable enough to understand it completely).

After initializing the apiObject I can make calls such as:

$results =$api->getTrack($elem, 'sevendigital');$results = $api->getAlbum($elem, 'sevendigital');


The getTrack and getAlbum are not full functions but rather gets a router call and then resolved with the necessary parameter in place.

Here are the two classes that form the API:

/**
* Music Story API Class
*/
class MusicStoryApi
{

/**
* Consumer key
* @var string
*/
protected $ConsumerKey; /** * Consumer secret key * @var string */ protected$ConsumerSecret;

/**
* Token key
* @var string
*/
protected $AccessToken; /** * Token secret key * @var string */ protected$TokenSecret;

/**
* Supported formats
* @var array
*/
private $formats = array('json', 'xml'); /** * API url * @var string */ protected$url_api = 'http://api.music-story.com/';

/**
* Sign methods
* @var array
*/
protected $sign_methods = array('sha1'); // Class errors const E_FORMAT = 'Unknown format'; const E_NO_CONSUMER_KEY = 'Empty consumer key'; const E_NO_CONSUMER_SECRET = 'Empty consumer secret key'; const E_NO_URL = 'Empty url'; const E_SIGN_METHOD = 'Unsupported signature method'; const E_UNKNOWN_METHOD = 'Unkown method'; const E_MISSING_PARAMETER = 'Missing parameter'; const E_UNKNOWN_CONNECTOR = 'Unknown connector'; const E_UNKNOWN_OBJECT = 'Unknown object'; /** * Constructor * @param string$consumer_key Consumer key
* @param string $consumer_secret Consumer secret key * @param string$access_token Access token (optional)
* @param string $token_secret Token secret (optional) * @param string$version Version de l'API (optional)
*/
public function __construct($consumer_key = null,$consumer_secret = null, $access_token = null,$token_secret = null, $version = null) { if (!$consumer_key)
$this->getError(__function__, self::E_NO_CONSUMER_KEY); if (!$consumer_secret)
$this->getError(__function__, self::E_NO_CONSUMER_SECRET);$this->setConsumerKey($consumer_key);$this->setConsumerSecret($consumer_secret); if (!$access_token || !$token_secret) {$this->getToken();
} else {
$this->setAccessToken($access_token);
$this->setTokenSecret($token_secret);
}
$this->url_api = ($version) ? $this->url_api .$version . '/' : $this->url_api; } /** * Router to setKey, getObject and searchObject methods * @param string$method Method name
* @param array $args Arguments * @return mixed (MusicStoryObject/MusicStoryObjects) */ public function __call($method, $args) { if (strpos($method, 'search') !== false)
return $this->searchObject(str_replace('search', '',$method), count($args) ?$args[0] : array(), isset($args[1]) ?$args[1] : null, isset($args[2]) ?$args[2] : null);
if (strpos($method, 'get') !== false) return$this->getObject(str_replace('get', '', $method), count($args) ? $args[0] : null, (isset($args[1]) && is_array($args[1])) ?$args[1] : array(), (isset($args[1]) && !is_array($args[1])) ? $args[1] : null); if (strpos($method, 'set') !== false)
$this->setKey(str_replace('set', '',$method), $args[0]); else$this->getError(__function__, self::E_UNKNOWN_METHOD);
}

/**
* Set consumer keys and token keys
* @param string $key Key name * @param string$val Key value
*/
protected function setKey($key,$val) {
$this->{$key} = $val; } /** * Get Music Story object by id * @param string$object Object name
* @param string/int $id Object id * @return MusicStoryObject */ protected function getObject($object, $id,$fields = array(), $partner = null) { if (!isset($id) || is_array($id))$this->getError(__function__, self::E_MISSING_PARAMETER);
$url =$this->setFormat($this->url_api . strtolower($partner ? $partner :$object) . '/' . $id . ($partner ? ('/' . strtolower($object)) : ''), 'json');$params = array('oauth_consumer_key' => $this->ConsumerKey, 'oauth_token' =>$this->AccessToken);
if (count($fields)) {$params['fields'] = implode(',', $fields); }$signature = $this->sign($url, $params);$signed_url = $url . '?' . (count($fields) ? $this->normalize_params(implode(',',$fields) . '&', false) : '') . $this->normalize_params($params, false) . 'oauth_consumer_key=' . $this->ConsumerKey . '&oauth_token=' .$this->AccessToken . '&oauth_signature=' . $this->rawurlencode_rfc3986($signature);
$result =$this->request($signed_url, true); return$this->constructResult($result,$object, $partner ? true : false); } /** * Search Music Story objects * @param string$object Object name
* @param array $filters Search filters * @param int$page Page number (optional)
* @param int $count Number of items per page (optional) * @return MusicStoryObjects */ protected function searchObject($object, $filters,$page = false, $count = false) {$filters = $this->getFields($filters);
if ($page)$filters['page'] = (string) $page; if ($count)
$filters['pageCount'] = (string)$count;
$url =$this->url_api . strtolower($object) . '/search';$url = $this->setFormat($url, 'json');
$params = array_merge($filters, array('oauth_consumer_key' => $this->ConsumerKey, 'oauth_token' =>$this->AccessToken));
$signature =$this->sign($url,$params);
$signed_url =$url . '?' . $this->normalize_params($filters, false) . '&oauth_consumer_key=' . $this->ConsumerKey . '&oauth_token=' .$this->AccessToken . '&oauth_signature=' . $this->rawurlencode_rfc3986($signature);
$result =$this->request($signed_url, true); if ($object == 'biographies')
$object = 'biography'; else if (substr($object, strlen($object) - 1, 1) == 's')$object = substr($object, 0, strlen($object) - 1);
return $this->constructResult($result, $object, true); } /** * Get new tokens * @param string$consumer_key Consumer key (optional)
* @param string $consumer_secret Consumer secret (optional) * @return array */ public function getToken($consumer_key = null, $consumer_secret = null) {$consumer_key = $consumer_key ?$consumer_key : $this->ConsumerKey;$consumer_secret = $consumer_secret ?$consumer_secret : $this->ConsumerSecret; if (!$consumer_key)
$this->getError(__function__, self::E_NO_CONSUMER_KEY); if (!$consumer_secret)
$this->getError(__function__, self::E_NO_CONSUMER_SECRET);$url = $this->setFormat($this->url_api . 'oauth/request_token', 'json');
$oauth_signature =$this->sign($url, array('oauth_consumer_key' =>$consumer_key), 'GET', true);
$response =$this->request($url . '?oauth_consumer_key=' .$consumer_key . '&oauth_signature=' . $oauth_signature, true);$this->setAccessToken($response['data']['token']);$this->setTokenSecret($response['data']['token_secret']); return array('access_token' =>$response['data']['token'], 'token_secret' => $response['data']['token_secret']); } /** * Set new tokens * @param string$access_token Access token
* @param string $token_secret Secret token */ public function setToken($access_token, $token_secret) { if (!isset($access_token) || !isset($token_secret) || !$access_token || !$token_secret)$this->getError(__function__, self::E_MISSING_PARAMETER);
$this->setAccessToken($access_token);
$this->setTokenSecret($token_secret);
}

/**
* Return oauth signature
* @param string $url Request url * @param array$params Request parameters
* @param string $method Http method * @param boolean$token_request The request is the token request
* @return string
*/
public function sign($url,$params, $method = 'GET',$token_request = false) {
if (!$url)$this->getError(__function__, self::E_NO_URL);
if (!$token_request && (!$this->AccessToken || !$this->TokenSecret))$this->getToken();
$sign_method = (isset($params['oauth_signature_method'])) ? $params['oauth_signature_method'] : 'sha1'; if (!in_array($sign_method, $this->sign_methods))$this->getError(__function__, self::E_SIGN_METHOD);
$params =$params ? $params : array();$method = strtoupper($method);$normalized_params = $this->normalize_params($params);
$base_signature =$this->get_base_signature($method,$url, $normalized_params);$encrypt_key = $this->getEncryptKey();$signature = base64_encode(hash_hmac($sign_method,$base_signature, $encrypt_key, true)); return$signature;
}

/**
* Return parameters into string format
* @param array $params Parameters * @param boolean$rfc3986 RFC3986 url encode
* @return string
*/
protected function normalize_params($params,$rfc3986 = true) {
ksort($params);$normalized = null;
foreach ($params as$attr => $val) {$val = str_replace(' ', '+', $val); if ($normalized)
$normalized .= '&';$normalized.=($rfc3986 ?$this->rawurlencode_rfc3986($attr) : rawurlencode($attr)) . '=' . ($rfc3986 ?$this->rawurlencode_rfc3986($val) : rawurlencode($val));
}
return $normalized; } /** * Set the "fields" parameter to be able to filter through facultative fields * @param array$filters filters
* @return array $filters completed filters */ protected function getFields($filters) {
if (!$filters)$filters = array();
$fields = ''; /* foreach($filters as $attr=>$val){
$fields.=(strlen($fields)?',':'').$attr; } */ return strlen($fields) ? array_merge($filters, array('fields' =>$fields)) : $filters; } /** * Encode url * @param string$input Url
* @return string
*/
protected function rawurlencode_rfc3986($input) { return str_replace('%7E', '~', str_replace('+', ' ', urlencode($input)));
}

/**
* Set request format into url
* @param string $url Url * @param string$format Format
* @return string
*/
protected function setFormat($url,$format = null) {
if (!in_array($format,$this->formats))
$this->getError(__function__, self::E_FORMAT); if ($format) {
foreach ($this->formats as$f) {
$url = str_replace('.' .$f, '', $url); }$url = (strpos($url, '?')) ? str_replace('?', '.' .$format . '?', $url) :$url . '.' . $format; } return$url;
}

/**
* Make an API request
* @param string $url Request url * @param boolean$parse Return result or parsed result
* @return mixed (string/array)
*/
public function request($url,$parse = false) {
$ch = curl_init(); curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
curl_setopt($ch, CURLOPT_URL,$url);
$answer = curl_exec($ch);
$format = 'xml'; foreach ($this->formats as $f) {$format = (strpos($url, '.' .$f)) ? $f :$format;
}
return $parse ?$this->parse($answer,$format) : $answer; } /** * Get signature base * @param string$method Http method
* @param string $url Url * @param string$normalized_params Parameters string
* @return string
*/
private function get_base_signature($method,$url, $normalized_params) {$matches = array();
preg_match('@^(http://|https://)?([^/]+)(.*)$@i',$url, $matches);$url = strtolower($matches[1] .$matches[2]) . $matches[3]; return$method . '&' . $this->rawurlencode_rfc3986($url) . '&' . $this->rawurlencode_rfc3986($normalized_params);
}

/**
* Get encrypt key
* @return string
*/
private function getEncryptKey() {
return $this->rawurlencode_rfc3986($this->ConsumerSecret) . '&' . $this->rawurlencode_rfc3986($this->TokenSecret);
}

/**
* Parse an API result
* @param string $data_string API result * @param string$format API result format
* @return array
*/
public function parse($data_string,$format = 'xml') {
switch ($format) { case 'xml':$data_tmp = simplexml_load_string($data_string);$data = get_object_vars($data_tmp);$results_data = $data['data'];$objects = array();
foreach ($results_data as$res) {
$data_vars = get_object_vars($res);
$object = array(); foreach ($data_vars as $item =>$value) {
if (is_object($value))$value = null;
$object[$item] = $value; }$objects[] = $object; }$data['data'] = $objects; break; case 'json':$data = json_decode($data_string, true); break; default:$this->getError(__function__, self::E_FORMAT);
}
return $data; } /** * Get exception * @param string$f Function name
* @param string $err Error Name * @throws Exception */ protected function getError($f, $err) { throw new Exception('Class error in method "' .$f . '": ' . $err); } /** * Transform parsed result into MusicStoryObject or MusicStoryObjects iterator * @param array$result Parsed result
* @param string $name Object(s) name * @param boolean$list Is result an object or a list of objects?
* @return mixed (MusicStoryObject/MusicStoryObjects)
*/
protected function constructResult($result,$name, $list = false) { if (isset($result['error'])) {
throw new Exception('API returned the following error: "' . $result['error']['message'] . '"'); } if (!$list)
$res = (isset($result['id'])) ? new MusicStoryObject($result,$name, array('ConsumerKey' => $this->ConsumerKey, 'ConsumerSecret' =>$this->ConsumerSecret, 'AccessToken' => $this->AccessToken, 'TokenSecret' =>$this->TokenSecret)) : null;
else {
$items = array(); if (isset($result['data'])) {
foreach ($result['data'] as$data) {
$items[] = new MusicStoryObject($data, $name, array('ConsumerKey' =>$this->ConsumerKey, 'ConsumerSecret' => $this->ConsumerSecret, 'AccessToken' =>$this->AccessToken, 'TokenSecret' => $this->TokenSecret)); } }$res = new MusicStoryObjects($items, isset($result['count']) ? $result['count'] : 1, isset($result['pageCount']) ? $result['pageCount'] : 1, isset($result['currentPage']) ? $result['currentPage'] : 1); } return$res;
}

}

/**
* Music Story Object Class
*/
class MusicStoryObject extends MusicStoryApi {

/**
* Object name
* @var string
*/
private $_object_name; /** * Constructor * @param array$api_result Parsed result
* @param string $name Object name * @param array$keys Consumer keys and token keys values
*/
public function __construct($api_result,$name, $keys) { foreach ($keys as $key =>$val)
$this->setKey($key, $val);$this->_object_name = $name; foreach ($api_result as $key =>$val)
if ($key != 'version' &&$key != 'code')
$this->{$key} = $val; } /** * Router to getConnector method * @param string$method Method name
* @param array $args Arguments * @return MusicStoryObjects */ public function __call($method, $args) { if (strpos($method, 'get') !== false) {
return $this->getConnector(str_replace('get', '',$method), count($args) ?$args[0] : array(), isset($args[1]) ?$args[1] : null, isset($args[2]) ?$args[2] : null);
} else
$this->getError(__function__, self::E_UNKNOWN_METHOD); } /** * Get connector result * @param string$connector Connector name
* @param array $filters Search filters * @param int$page Page number (optional)
* @param int $count Items per page (optional) * @return MusicStoryObjects */ public function getConnector($connector, $filters,$page = false, $count = false) { if ($page)
$filters['page'] = (string)$page;
if ($count)$filters['pageCount'] = (string) $count;$url = $this->url_api . strtolower($this->_object_name) . '/' . $this->id . '/' . strtolower($connector);
$url =$this->setFormat($url, 'json');$params = array_merge($filters, array('oauth_consumer_key' =>$this->ConsumerKey, 'oauth_token' => $this->AccessToken));$signature = $this->sign($url, $params);$signed_url = $url . '?' .$this->normalize_params($filters, false) . '&oauth_consumer_key=' .$this->ConsumerKey . '&oauth_token=' . $this->AccessToken . '&oauth_signature=' .$this->rawurlencode_rfc3986($signature);$result = $this->request($signed_url, true);
if ($connector == 'biographies')$connector = 'biography';
else if (substr($connector, strlen($connector) - 1, 1) == 's')
$connector = substr($connector, 0, strlen($connector) - 1); return$this->constructResult($result,$connector, true);
}

}


Questions:

1. Would it be smart to decouple the __call function and make smaller functions in order to do TDD?
2. is there any downside to using __call?
3. How would you refactor to avoid too many protected/private functions?
4. Would you replace the protected function by another mean so that you can have TDD on each functions (or a good amount of functions to TDD on)?

There is an major issue with __call, and that is mostly that it is impossible for any IDE to guess correctly what API methods would even be valid names.

But this is not limited to IDE support, since you don't have an interface which models the API, you can't even implement a stub for the sake of integration tests on the calling site.

Looking at this not as a developer of the API class, but more as a user of the API, I have absolutely no idea how to invoke the API wrapper, and how to integrate it. I would either need external documentation - which means your code doesn't fulfill the basic requirement of being self-documenting - or I would need to probe against the live API.

As for the long list of private and protected functions: In theory you could offload them via the trait system in PHP, but that one unfortunately has issues of its own, regarding not loosing track.

You've got a lot of actually static/pure functions in there which aren't marked as such, and which could easily be outsourced into a utility library, but that still leaves a lot of clutter.

So let's rephrase the question: Why do the private and protected functions annoy you so much?

Because they are not of interest to the enduser who is going to use your API. And neither is the specific implementation of the public ones.

This is currently hard to distinguish for multiple reasons. A major violation of good style is, that you mixed private, public and protected functions however you liked.

There is no "break" in there at which the interested user could just stop looking, but anyone trying to grasp the internals is forced to read the entire file in search of the publicly accessible interface.

All the user ultimately expects, is a clean interface with strong and explicit type hints and remarks regarding side effects and state transitions, as well as the proper declaration of possible error values and states.

Error handling. That's another problem with your API.

You are throwing generic Exception objects in multiple locations. (By the way via a method called getError, why that? That name is misleading!)

You are not even inheriting from Exception, to provide an error handling which can in any way be programmatically traced back to an problem with the API, but the only trace to that is hidden in the message.

Plus you are also somewhat trying to emulate a stack trace here, but that's not what the enduser expects from an message. He wants to know why it failed - where it failed is something he get much easier and detailed from Exception::getTraceAsString.

But that's not even what exceptions are made for. You are solely using them for cases where the integration of your API is wrong, not for handling (recoverable) exceptions. Use PHPs builtin error handling system for this cause. This works much better than relying on an untyped exception not getting caught and swallowed eventually by some component higher up in the stack.

E.g. replace the exceptions, respectively the current calls to getError() with:

trigger_error("Mandatory parameter was empty.", E_USER_ERROR);


In return for the abuse of the exceptions, there is absolutely no error handling at all for server side errors, or not necessarily missing, but malformed parameters.

There is no error handling in MusicStoreApi::request(), you are not even checking that curl did return any valid result at all.

Neither are you handling the case that the server could in any way possibly return an empty result set. The only hint of error handling is in MusicStoreApi::constructResult(), which is both too late, and completely insufficient.

The only proper way to handle remote connections like this, is not to blacklist specific error conditions, but to whitelist a successful request.

You know what also smells?

Duplicate code.

$url =$this->setFormat($url, 'json');$params = array_merge($filters, array('oauth_consumer_key' =>$this->ConsumerKey, 'oauth_token' => $this->AccessToken));$signature = $this->sign($url, $params);$signed_url = $url . '?' .$this->normalize_params($filters, false) . '&oauth_consumer_key=' .$this->ConsumerKey . '&oauth_token=' . $this->AccessToken . '&oauth_signature=' .$this->rawurlencode_rfc3986($signature);$result = $this->request($signed_url, true);


This fragment is - with slight variations to the first parameter of MusicStoreApi::setFormat(), copied and pasted all over the place. I'm counting currently 5 copies of this fragment, and in all 5 locations, it's the major cause for bloat in the respective scope.

But not only duplicated code is a problem with this - if we exhibit the fragment closely, we see that MusicStoreApi::request() is exclusively called with the second parameter set to true, and MusicStoreApi::setFormat() exclusively with the second parameter set to "json". Which means the generalization of these two functions is just plain inadequate for your code and only add to bloat.

If you don't need a functionality, not even in the foreseeable future, don't add it. You still got to maintain it, and the chance of accidentally introducing a bug in such an inactive code path is pretty high. Even with TDD, you are not going to reach a proper code quality in inactive sections, and by he time you put it back into use, it's most likely defective.

• well done in your explanation. However could have used some code snippet for the Error exception part for completeness – azngunit81 May 9 '16 at 16:38

1. Would it be smart to decouple the __call function and make smaller functions in order to do TDD?

I'm not exactly sure what you meant by decoupling here. But whatever helps you doing TDD, I'd say go for it.

1. is there any downside to using __call?

It seems indeed magical (as you feared in the intro description). If you don't need it, don't use it. If you can replace it with something better, less magical, do it.

1. How would you refactor to avoid too many protected/private functions?

What's wrong with protected/private functions? Hiding low-level implementation details is a good thing. There's nothing wrong with a large number of small functions that do one thing and do it well.

1. Would you replace the protected function by another mean so that you can have TDD on each functions (or a good amount of functions to TDD on)?

Only if necessary. There's no need to test every helper method individually. I think it's best to test a program through its public API methods. If you test all the functionality of the public API, then all the private methods should get covered, naturally.

The only exception is the case of complex helper methods that would be too difficult to test through the public API, and a lot easier to test directly. It's acceptable to make complex helper methods protected for easier testing.

Your main questions concern testing, so it's a bit curious that you did not include your test cases.

### getError -> throwError

getError is a misleading name. The "get" prefix is usually used for functions that simply return the value of private fields (and are called "getter").

This function in fact throws an exception. Looking at the other code calling this function, it's sometimes not at all obvious that an exception will be thrown and the rest of the function in the caller won't get executed. It would be better to make this obvious, by renaming the function to throwError or similar.

### Excessively long lines

Some of the lines are very long, and force readers to scroll to the right. I'm talking about these, for example:

$signed_url =$url . '?' . (count($fields) ?$this->normalize_params(implode(',', $fields) . '&', false) : '') .$this->normalize_params($params, false) . 'oauth_consumer_key=' .$this->ConsumerKey . '&oauth_token=' . $this->AccessToken . '&oauth_signature=' .$this->rawurlencode_rfc3986($signature);$signed_url = $url . '?' .$this->normalize_params($filters, false) . '&oauth_consumer_key=' .$this->ConsumerKey . '&oauth_token=' . $this->AccessToken . '&oauth_signature=' .$this->rawurlencode_rfc3986($signature);  Code that's not visible generally has more bugs, as it naturally gets read less. It would be good to refactor these lines to make them shorter, ideally by adding more helper functions. These examples seem particularly good candidates for that, as they have repeated elements. (In fact the second line appears in your code twice, verbatim.) There are other very long lines, try to review and shorten them all. ### Overusing the ternary operator ?: The ternary operator is overused. It hurts readability, and sometimes causes repeated evaluations. Take for example: return$this->searchObject(str_replace('search', '', $method), count($args) ? $args[0] : array(), isset($args[1]) ? $args[1] : null, isset($args[2]) ? $args[2] : null);  The form isset(x) ? x : null appears many times in the code in general. At the minimum, you could create a helper function for that. Or, depending on your version of PHP, you could use the coalesce operators ?: or ??. See this related post on Stack Overflow for examples. As an example of unnecessary repeated evaluation, on this line the value of $partner is evaluated twice, in two ternary expressions:

$url =$this->setFormat($this->url_api . strtolower($partner ? $partner :$object) . '/' . $id . ($partner ? ('/' . strtolower($object)) : ''), 'json');  It would be better and more readable to just spell this out as regular if-else: if ($partner) {
$url =$this->setFormat($this->url_api . strtolower($partner) . '/' . $id . '/' . strtolower($object), 'json');
} else {
$url =$this->setFormat($this->url_api . strtolower($object) . '/' . $id, 'json'); }  Sure, there is repetition here. But now it's a bit easier to see the variable part in the middle that can be extracted to a helper method, refactoring to: $url = $this->setFormat($this->url_api . relpath($partner,$object), 'json');


The relpath helper will have:

$object = strtolower($object);
if ($partner) { return strtolower($partner) . '/' . $id . '/' .$object;
}
return $object . '/' .$id;


At this point, if you really want, you could actually bring back a ternary for this last step. Now the code is short enough that the ternary will not hurt the readability too much, and there will be no more repeated evaluation.

This is just one example of the overly long, complicated lines with multiple ternary expressions. I suggest to review them all, and improve similarly if possible.

• this is an API adapter function so __call when passed as an object will called an endpoint of the url that will invoke a RESTful API request. like $api->artists() will work, $api->albums also works but \$api->fooBar will yield a 404 because the other end won't know. I found out that this class is very short-handed (via magic functions) hence why I wanted it decoupled (or rather asking if it should or not) – azngunit81 May 5 '16 at 21:54
• I did not include tests cases because of the magic functions that magically calls the necessary URLs for me therefore causing me not to be able to know if I should leave this class alone or should I refactor to be testable. – azngunit81 May 5 '16 at 22:45