2
\$\begingroup\$

I'm implementing the Hotel Price List API from Skyscanner. This requires a first call to the service to implement a session followed by 1 or more calls to the service to get the actual results. I've used curl to implement this and got a working solution, I just wonder if this solution can be improved.

What I have:

public static function CallAPI($method, $url, $data = false)
  {
    $curl = curl_init();

    switch ($method)
    {
        case "POST":
            curl_setopt($curl, CURLOPT_POST, 1);

            if ($data)
                curl_setopt($curl, CURLOPT_POSTFIELDS, $data);
            break;
        case "PUT":
            curl_setopt($curl, CURLOPT_PUT, 1);
            break;
        default:
            if ($data)
                $url = sprintf("%s?%s", $url, http_build_query($data));
    }

    curl_setopt($curl, CURLOPT_URL, $url);
    curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);
    curl_setopt($curl, CURLOPT_VERBOSE, 1);
    curl_setopt($curl, CURLOPT_HEADER, 1);

    // Request json
    curl_setopt($curl, CURLOPT_HTTPHEADER, array(
    'Accept: application/json'));

    $result = curl_exec($curl);

    $header_size = curl_getinfo($curl, CURLINFO_HEADER_SIZE);
    $header = substr($result, 0, $header_size);
    $body = substr($result, $header_size);

    $APIResult['error'] = curl_error($curl);
    $APIResult['info'] = curl_getinfo($curl);
    $APIResult['header'] = $header;
    $APIResult['body'] = $body;

    curl_close($curl);

    return $APIResult;
}


  public function GetHotels($nodeId) {

    $node = Node::find($nodeId);

    $apiKey = 'prtl6749387986743898559646983194'; // This is the test key

    $currency = $this->request->session()->get('currency');
    $locale = Lang::locale();
    $market = $this->request->session()->get('countryCode');
    $entityid = $node->lat . ',' . $node->lng . '-latlong';
    $checkindate = '2016-12-17';
    $checkoutdate = '2016-12-18';
    $guests = 2;
    $rooms = 1;

    $url = 'http://partners.api.skyscanner.net/apiservices/hotels/liveprices/v3/' . $market . '/'. $currency . '/'. $locale . '/'. $entityid . '/'. $checkindate . '/'. $checkoutdate . '/'. $guests . '/'. $rooms;

    // First call to initiate session, no actual results retrieved
    $APIResult = APIServices::CallAPI('GET', $url, array("apiKey" => $apiKey));


    $debugResult = $APIResult['body'];

    if (!empty($APIResult['info']['redirect_url']) ) {
        $APIResult =  APIServices::CallAPI('GET', $APIResult['info']['redirect_url']);
        $json = json_decode($APIResult['body'],true);

        //Concatenate the json results
        $result = $APIResult['body'];


        while ($json['status'] <> 'COMPLETE') {
            $APIResult =  APIServices::CallAPI('GET', $APIResult['info']['redirect_url']);
            $json = json_decode($APIResult['result'],true);
           //Concatenate the json results
           $result = $result . $APIResult['body'];
        }
    }

  }

I know curl is able to automatically follow redirect URL's but I don't know if this would be better? The current solution to concat the json results in the loop works but is it the proper way to combine multiple JSON results in one? Any other thoughts, concerns? This is the first time for me to call a RESTful API.

\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
public static function CallAPI($method, $url, $data = false)
  • Use lowercase first letter for method and function names. callAPI.
  • Use null rather than false as default value for $data as this more truly represents what is happening (no value has been passed).
  • Once in your method, you are not doing anything to validate that the parameters passed are as expected. For example, $method and $url should, at a minimum be validated as non-zero length strings. Perhaps URL also needs to be validated as valid URL and method to be one of set of values you are expecting (i.e. POST,PUT,GET,DELETE,etc.). $data should be validated as null or non-zero length string. If these validations do not pass, you should fail out of this method (perhaps with InvalidArgumentException or similar) and not ever attempt to make a curl call.

$curl = curl_init();
  • Here, you just assume your curl_init() works. You should should check for failure and handle appropriately (log error, throw Exception, etc.).
  • Consider moving your curl initialization sequence into its own static method which would initiate and store the curl handle on the class, making it available to the callAPI method. There is no reason for you to continually set up and tear down curl handles for each API call. You probably only need to do this once (changing settings on handle as appropriate.
  • This centrally stored handle will also be something you will need if you want to be able to optimize your code to making API calls in parallel (more on this below).

switch ($method)
{
    case "POST":
        curl_setopt($curl, CURLOPT_POST, 1);

        if ($data)
            curl_setopt($curl, CURLOPT_POSTFIELDS, $data);
        break;
    case "PUT":
        curl_setopt($curl, CURLOPT_PUT, 1);
        break;
    default:
        if ($data)
            $url = sprintf("%s?%s", $url, http_build_query($data));
}
  • Rather than having logic here in switch consider actually having distinct get(), post(), put(), delete(), etc. methods on your class. Each method can make necessary config settings on curl handle and then call the central method which makes the API call and assembles the response data structure.

An incomplete implementation for this might be structured as follows:

public static function get($url, $data) {
    // make setting changes to curl handle here
    // then call call API method, which at this point is just a 
    // request/repsonse handler
    return self::callAPI($url, $data);
}

curl_setopt($curl, CURLOPT_RETURNTRANSFER, 1);
curl_setopt($curl, CURLOPT_VERBOSE, 1);
curl_setopt($curl, CURLOPT_HEADER, 1);

Consider moving these option settings to common initialization method as noted previously.


// Request json
curl_setopt($curl, CURLOPT_HTTPHEADER, array(
'Accept: application/json'));

Again, probably best set during handle initialization


$APIResult['error'] = curl_error($curl);
$APIResult['info'] = curl_getinfo($curl);
$APIResult['header'] = $header;
$APIResult['body'] = $body;

Consider building this as object rather than associative array, as this really seems to be more of a set of object data rather that a hashmap type of data structure. You might over time also find a need to add behaviors around these properties such that having your own APIResult class or similar makes sense. Passing around an object dependency with known properties and behaviors might be preferable to just passing around a data structure.


public function GetHotels($nodeId) {
  • Again consider naming getHotels() and adding validation for parameter.
  • This method doesn't return anything! What is the point of having this public method if it doesn't do anything for the caller?

$node = Node::find($nodeId);

Is $node a meaningful variable name here? $coordinates perhaps?


$apiKey = 'prtl6749387986743898559646983194'; // This is the test key

This should not be defined here in this method. At a minimum it should be a constant on this class, but would be better still if this was stored in application configuration and not in this code at all.


$checkindate = '2016-12-17';
$checkoutdate = '2016-12-18';
$guests = 2;
$rooms = 1;

Why are these hard-coded?


$url =
'http://partners.api.skyscanner.net/apiservices/hotels/liveprices/v3/'
. $market . '/'. $currency . '/'. $locale . '/'. $entityid . '/'.
$checkindate . '/'. $checkoutdate . '/'. $guests . '/'. $rooms;

Consider having URL base as class constant and having a method to build the full URL


// First call to initiate session, no actual results retrieved
$APIResult = APIServices::CallAPI('GET', $url, array("apiKey" => $apiKey));

If you incorporate other suggestions, this might look like:

$APIResult = APIServices::get($url, array("apiKey" => $apiKey));

$debugResult = $APIResult['body'];

What is purpose of this?


if (!empty($APIResult['info']['redirect_url']) ) {
    $APIResult =  APIServices::CallAPI('GET', $APIResult['info']['redirect_url']);

If you know your API is going to give you redirect, just set curl to follow. Don't write a bunch of nested code to handle these redirects. What if there are two redirects here? Your code fails.


    $json = json_decode($APIResult['body'],true);

Why would you call this variable $json when it does not contain JSON, but rather a data structure resulting from JSON deserialization? Call the variable something that is meaningful to what it is actually storing. What if json_decode() fails (i.e. server sent you something that is improperly formatted)?


    //Concatenate the json results
    $result = $APIResult['body'];

Why are you continuing to work with the raw JSON string here? You already have a meaningful data structure from the previous line of code. Concatenating JSON together DOES NOT WORK, as you will get an invalid JSON string. JSON is ONLY used for data transport, not as a mechanism to build a data structure. Focus on building an appropriate data structure and then serialize to JSON for delivery.


    while ($json['status'] <> 'COMPLETE') {
        $APIResult =  APIServices::CallAPI('GET', $APIResult['info']['redirect_url']);
        $json = json_decode($APIResult['result'],true);
       //Concatenate the json results
       $result = $result . $APIResult['body'];
    }

And last but not least. I think you might strongly consider making these API calls in parallel, as that will be the fastest way for this script to operate (though it would consume more memory as a trade-off). Take a look at curl_multi_* functions (or a library which can better handle your curl operations rather than you coding them manually). This relates back to earlier point of needing to store curl handle on the class such that each individual call to callAPI method (or the wrapper methods). To implement parallel calls you would likely need to store curl_multi handle on the class, with each iteration of this loop adding a new curl handle to the multi-handle and returning data from each API call to be aggregated by calling getHotels() method.

I will link you to a lightweight class I had written at one point to manage multi_curl handles geared towards a RESTful use case. Feel free to use in your application or for inspiration.

https://github.com/mikecbrant/php-rest-client


In general, and without seeing your full class implementation, I am worried that your class is doing too much. Why would one class need to know all of the following?

  • How to initiate can configure cURL calls. Ideally this could take a valid cURL wrapper object as a dependency and not have to do anything other than formulating the request and handling the response.
  • The low-level details of formulating requests against this specific API (i.e. URL naming, redirection handling, etc.)
  • The logic for deriving localization, geography, etc. based on the request/requestor.
  • The specifics of performing a hotel search.
  • The specifics of getting details for an individual hotel
\$\endgroup\$
3
  • \$\begingroup\$ you are suggesting to use curl_multi* functions. If I understand them correctly they will make the calls in parallel. Knowing that the Skyscanner interface (link) will give me a redirect URL with each poll I do as long as I need to keep polling I'm not quite sure if this is a possible solution. I don't know all the URL's from the start, I rather get the next URL with each subsequent poll I do. \$\endgroup\$
    – DiscoFever
    Sep 23, 2016 at 7:49
  • \$\begingroup\$ @KrisCaluwaerts ok in that case making parallel request might not be a viable approach. \$\endgroup\$
    – Mike Brant
    Sep 23, 2016 at 11:25
  • \$\begingroup\$ @KrisCaluwaerts You might also consider leveraging a queueing mechanism which might allow you to work in parallel as long as you have one authoritative place in memory to access the list of API endpoints that need to be accessed. \$\endgroup\$
    – Mike Brant
    Oct 4, 2016 at 13:20

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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