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 st

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,

StackExchange Code Review Q#123274, answer score: 5