Smartling / api-sdk-php

SDK for integrating with the Smartling API. The Smartling API allows developers to upload language specific resource files and download the translations of those files for easy integration within their application. http://docs.smartling.com
Apache License 2.0
6 stars 15 forks source link

Refactor return value of Jobs API functions #80

Open dimitrystd opened 7 years ago

dimitrystd commented 7 years ago

I noticed that all functions that call BaseApiAbstract->sendRequest() return bool. An example,

/**
 * Returns a job.
 *
 * @param string $jobId
 * @return bool
 */
public function getJob($jobId)
{
    $requestData = $this->getDefaultRequestData('query', []);
    $request = $this->prepareHttpRequest('jobs/' . $jobId, $requestData, self::HTTP_METHOD_GET);
    return $this->sendRequest($request);
}

But if you look at sendRequest() more close that it can return only:

In example above we don't expect data, thus it means our function can return only true. Such "contract" may confuse end-developer. If i see return bool then i add if () then statement in code. But actually i should add try catch.

So we should review SDK methods and remove return statement if it returns only a single value. Let's refactor only Jobs API because we don't use it yet.

DoD:

PavelLoparev commented 7 years ago

@dimitrystd

In example above we don't expect data, thus it means our function can return only true.

I believe you meant cancelJob or authorizeJob method - only they return boolean true. Because getJob actually should return an array with a job info.

I've removed return statement from cancelJob and authorizeJob methods and updated comments for all the JobaAPI methods.

dimitrystd commented 7 years ago

Thanks, probably added getJob accidentally.