1
\$\begingroup\$

I have this code but it takes over a minute to run. I'm looking at around 400 entries being brought back for the first loop. Any ideas on how it could be improved for speed?

<?php

    $context = stream_context_create(array(
        'http' => array(
            'method' => 'GET',
            'header' => "Authorization: Basic abc123\r\nContent-Type: application/json\r\n"//,'content' => json_encode($postData)
        )
    ));

    // Send the request
    $response = file_get_contents('https://localhost', FALSE, $context);

    // Check for errors
    if($response === FALSE){
        die('Error');
    }
    // Decode the response
    $responseData = json_decode($response, TRUE);

    // Print the date from the response
    foreach($responseData as $row) {
       foreach($row as $k) {
             $response = file_get_contents('https://localhost/answers'.$k['id'], FALSE, $context);
             $responseDataAns = json_decode($response, TRUE);
                 echo $responseDataAns['summary']." / ".$responseDataAns['solution']."\r\n";
       }
    }
    ?>

This is the REST API I am using specifically. This is used to retrieve all answers then this to get an instance.

I want to use the REST API to list all questions, allow someone to go into a question and then update it.

\$\endgroup\$
5
  • \$\begingroup\$ What is in the $response? What are those rows? Also, why are you using ob_flush();? I don't see ob_start(); anywhere. Also, wouldn't be easier to make a single cache file to which you would be storing the data you are now generating every single time? Its kinda hard to know what this actually does and whats in the files. You have to provide a bit more to this question. \$\endgroup\$ Dec 8, 2016 at 13:21
  • \$\begingroup\$ You can execute the subsequent requests in parallel using curl multi exec. \$\endgroup\$ Dec 8, 2016 at 13:40
  • \$\begingroup\$ Updated with more info \$\endgroup\$
    – pee2pee
    Dec 8, 2016 at 16:50
  • \$\begingroup\$ I would say the API needs to be changed. If you know EVERY time that you will need to echo out a full set of "answer" information. Why not have the very first API call return your fully built-out data structure rather than using a bunch of subsequent requests to incrementally build the data? It looks like the API you provided has a method to work with collection of answers. \$\endgroup\$
    – Mike Brant
    Dec 8, 2016 at 18:09
  • \$\begingroup\$ The collection of answers only brings back the ID without any question/answer text \$\endgroup\$
    – pee2pee
    Dec 9, 2016 at 9:23

2 Answers 2

1
\$\begingroup\$

It seems to me that you are making multiple requests to 'https://localhost/answers'.$k['id'] inside a loop and that you have one route for each answers, such as:

https://localhost/answers100
https://localhost/answers101
https://localhost/answers102

My suggestion is to abandon that approach and combine them into one route that can handle multiple ids. For instance:

https://localhost/answers?id[]=100&id[]=101&id[]=102

Then grab the id like this:

$ids = $_GET['id'];

which is an array. Process each id one by one and combine the result and send as JSON. For instance:

{
    "100": {
        "summary": "",
        "solution": ""
    },
    "101": {
        "summary": "",
        "solution": ""
    },
    "102": {
        "summary": "",
        "solution": ""
    }
}

So, instead of making 400 requests, you only need to make 1 request. Much faster.

\$\endgroup\$
0
\$\begingroup\$

To answer your question about performance: you are doing 400 requests to an external service. What did you expect? Even if your requests are returned in 30ms, it will still take you 2 minutes.


Now on to the codereview:

Coding style

Your coding style is artistic. Try to stick to a well known coding standard like psr-1 and psr-2. This will make your code easier to read for others and yourself.

Note that most IDE's come built-in with this code style and they do the formatting for you.

Seperation of concern

Your code lacks any kind of seperaration; Everything is written in one go, making it very hard to understand and/or maintain. Depending on your choice. Given the size of the code, creating a couple of functions might be usefull:

function retrieveAnswer($answerId); function retrieveAnswerList($answerId); function retrieveObject($endpoint);

this would clean up your code a bit:

``` $answers = retrieveAnswerList('/');

foreach($answers as $answerRow) {
   foreach($answerRow as $answerDefinition) {
         $answer = retrieveAnswer($answerDefinition['id']);
         printAnswer($answer);
   }
}

```

We now seperated your "business logic" from the actual implementations. This code doesn't care if the api returns json or xml or ...

Variable names

As seen above, I changed the variable names to have more meaning. Avariable called $responseData doesnt say anything. What kind of data? what kind of response? Do we even care the data came from a response? Choosing variables is hard, but it will make your code read more like natural language.

Comments

comments should explain the why, not the what. // Decode the response. Yes, I can see your are decoding something, thats what json_decode means. I know that. But why?

Small code remark

I love that you use contest + file_get_contents. Not many developers know that trick. But you can have an even neater syntax:

$context = stream_context_create(array( 'http' => array( 'method' => 'GET', 'header' => array( "Authorization" => "Basic abc123", "Content-Type" => "application/json", ), 'content' => json_encode($postData) ) ));

\$\endgroup\$

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.