0
\$\begingroup\$

I have a PHP cronjob script. I need to send requests and get responses from some external API.

So, there is code for the API class. In this class, I connect to the API and call public methods of this class from another class.

class Api {
private $authApi;
private $logger;

function __construct()
{
    global $_AUTH_API;
    $this->authApi = $_AUTH_API;
    $this->logger = new CustomLogs();
}

public function getBrands()
{
    $options = [
        'request' => 'get_brands',
    ];

    $brands = $this->requestApi($options);

    return $brands->rows;
}

public function get****() {}

/**
 * @param $data
 *
 * @return array|mixed|object
 * @throws Exception
 */
private function requestApi($data)
{
    $data = $this->authApi + $data;

    $data = [
        'data' => json_encode($data)
    ];

    $ch = curl_init();
    curl_setopt($ch, CURLOPT_URL, "https://api.com/api/");
    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER,FALSE);
    curl_setopt($ch, CURLOPT_SSL_VERIFYHOST,2);
    curl_setopt($ch, CURLOPT_POST, 1);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_POSTFIELDS, $data);

    $answer = json_decode(curl_exec($ch));
    curl_close($ch);

    if (empty($answer)) {
        $this->logger->log->error('Empty answer from API');
        throw new Exception('Empty answer from API');
    } else {
        if ($answer->success != true) {
            $this->logger->log->error('API error: ', (array) $answer );
            throw new Exception('API error: ' . $answer->message  );
        } else {
            return $answer;
        }
    }
}
}

How can different API responses be handled correctly? What should be written in Logs? Do I need to throw Exceptions? What should be returned to external code? (i.e. in getBrands())

What would you change in this code?

\$\endgroup\$

closed as off-topic by t3chb0t, vnp, Abbas, Incomputable, pacmaninbw Dec 15 '17 at 18:02

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – vnp, Abbas, Incomputable, pacmaninbw
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ what are "different API responses" actually? it's hard to answer such a question other than "handle them differently" \$\endgroup\$ – Your Common Sense Dec 12 '17 at 12:32
  • \$\begingroup\$ Sucess: true/false, empty or If API will send something unexpected. \$\endgroup\$ – Zhi V Dec 12 '17 at 12:40
  • \$\begingroup\$ I changed the title so that it describes what the code does per site goals: "State what your code does in your title, not your main concerns about it.". Feel free to give it a different title if there is something more appropriate. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 12 '17 at 17:09
2
\$\begingroup\$

First of all, it's a job for at least three separate functions (that should belong to the different classes at best, but that would be a different story), each throwing its own exceptions.

protected function sendRequest($data)
{
    $ch = curl_init();
    curl_setopt($ch, CURLOPT_URL, $this->apiUrl);
    curl_setopt($ch, CURLOPT_SSL_VERIFYPEER,FALSE);
    curl_setopt($ch, CURLOPT_SSL_VERIFYHOST,2);
    curl_setopt($ch, CURLOPT_POST, 1);
    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($ch, CURLOPT_POSTFIELDS, $data);
    $data = curl_exec($ch);
    if ($error = curl_error($ch)) {
        throw new Exception($error);
    }   
    curl_close($ch);
    return $data;
}
protected function jsonDecode($json)
{
    $data = json_decode($json);
    if (json_last_error()) {
        throw new Exception(json_last_error_msg());
    }
    return $data;
}
protected function requestApi($data)
{
    $data = $this->authApi + $data;
    $data = [
        'data' => json_encode($data)
    ];
    $response = $this->sendRequest($data);
    $answer = $this->jsonDecode($response);

    if (isset($answer->success) && $answer->success == true))) {
        return $answer;
    }
    if (isset($answer->message) {
        throw new \Exception('API error: ' . $answer->message  );
    } 
    throw new \Exception("Unknown API response: $response" );
}

As of the logger, I would call it in the exception handler/catch block and thus avoid repetitive calls.

\$\endgroup\$
  • \$\begingroup\$ Should jsonDecode() return $data? The OP proposed that change. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 12 '17 at 14:07
  • \$\begingroup\$ json_last_error_msg() returns "NO ERROR" and then throw Exception \$\endgroup\$ – Zhi V Dec 12 '17 at 15:07

Not the answer you're looking for? Browse other questions tagged or ask your own question.