Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor return value of Jobs API functions #80

Open
dimitrystd opened this issue Sep 7, 2017 · 2 comments
Open

Refactor return value of Jobs API functions #80

dimitrystd opened this issue Sep 7, 2017 · 2 comments
Assignees

Comments

@dimitrystd
Copy link
Contributor

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:

  • Some data (JSON or file body)
  • true
  • Exception

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:

  • Update following methods:
    • createJob
    • updateJob
    • cancelJob
    • listJobs
    • getJob
    • authorizeJob
    • addFileToJob
  • Update method documentation. Make it clear that it doesn't return value, but may throw exception
@throws \Smartling\Exceptions\SmartlingApiException
@PavelLoparev
Copy link
Contributor

PavelLoparev commented Sep 8, 2017

@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
Copy link
Contributor Author

Thanks, probably added getJob accidentally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants