1
\$\begingroup\$

I'm building a web app that pulls data from a ticketing system via REST API. I'm currently able to pull all of the data I need just fine but the loading process is really slow. I'd love any feedback regarding the optimization of my code.

<?php
include 'schedule.php';

$curl = curl_init();

foreach ($onsites as $value){

curl_setopt_array($curl, array(
    CURLOPT_URL => "https://portal.-----.com/v4_6_release/apis/3.0/service/tickets?conditions=id%3D$value",
    CURLOPT_RETURNTRANSFER => true,
    CURLOPT_ENCODING => "",
    CURLOPT_MAXREDIRS => 10,
    CURLOPT_TIMEOUT => 30,
    CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1,
    CURLOPT_CUSTOMREQUEST => "GET",
    CURLOPT_HTTPHEADER => array(
        "accept: application/json",
        "authorization: -----",
        "cache-control: no-cache",
        "postman-token: -----",
        "x-cw-usertype: integrator"
    )
));

$response = curl_exec($curl);
$err      = curl_error($curl);

if ($err) {
    echo "cURL Error #:" . $err;
} else {
    $response = json_decode($response, true);

    foreach ($response as $ticket) {            
        $ticket_number = $ticket['id'];
        $summary[]       = $ticket['summary'];
        $company[]       = $ticket['company']['name'];
        $site[]          = $ticket['siteName'];
        $addressLine1  = str_replace('# D', '#D', $ticket['addressLine1']);
        $addressLine2  = $ticket['addressLine2'];
        $city          = $ticket['city'];
        $state         = $ticket['stateIdentifier'];
        $zip           = $ticket['zip'];
        $contact[]       = $ticket['contact']['name'];
        $resources     = $ticket['resources'];
        if(empty($addressLine2)) { 
            $full_address[]   = "$addressLine1, $city, $state, $zip, USA";
        } else {
            $full_address[]   = "$addressLine1, $addressLine2, $city, $state, $zip, USA";
        }
    }
}
}

mysql_connect("localhost", "root", "") or die(mysql_error());
mysql_select_db("scheduler") or die(mysql_error());

if ($ticket_number = $onsites) {
$current_onsites = array();
$i = 0;

for ($i = 0; $i <= count($ticket_number); $i++) {
    @$current_onsites[$i]['id'] = $ticket_number[$i];
    @$current_onsites[$i]['text'] = $summary[$i];
    @$current_onsites[$i]['location'] = $location[$i];
    @$current_onsites[$i]['company'] = $company[$i];
    @$current_onsites[$i]['site'] = $site[$i];
    @$current_onsites[$i]['location'] = $full_address[$i];
    @$current_onsites[$i]['contact'] = $contact[$i];
    @$current_onsites[$i]['start_date'] = date('Y-m-d H:i:s', strtotime($startDate[$i]));
    @$current_onsites[$i]['end_date'] = date('Y-m-d H:i:s', strtotime($endDate[$i]));
    @$current_onsites[$i]['technician'] = $technician[$i];
    @$current_onsites[$i]['hours'] = $hours[$i];
    @$current_onsites[$i]['status'] = $status[$i];
    @$current_onsites[$i]['done'] = $doneFlag[$i];
    @$current_onsites[$i]['ownerFlag'] = $ownerFlag[$i];
    // Retrieve all the data from the db table
    $result = mysql_query('SELECT * FROM xy WHERE company LIKE "'.@$current_onsites[$i]['company'].'";')
        or die(mysql_error());  
    // store the record of the "example" table into $row
    $row = mysql_fetch_array( $result );    
    @$current_onsites[$i]['lat'] = $row['lat'];
    @$current_onsites[$i]['lng'] = $row['lng'];
}

print "<pre>";
print_r ($current_onsites);
print "</pre>";

$fp = fopen('results.json', 'w');
fwrite($fp, json_encode($current_onsites));
fclose($fp);
}

curl_close($curl);

?>
\$\endgroup\$
  • \$\begingroup\$ Where does $value come from? Is it a user-supplied parameter value? \$\endgroup\$ – 200_success Aug 1 '16 at 21:04
  • \$\begingroup\$ Have you measured whether the performance bottleneck is server-side or client-side? If it's server-side, then there's not much to be gained by optimizing the client. \$\endgroup\$ – 200_success Aug 1 '16 at 21:04
  • \$\begingroup\$ 200_success, $value is user supplied. It's just an array of values to be iterated through for each request. Also, how can I determine where the bottleneck might be? I'm using Chrome and am more or less familiar with the DevTools but not sure how to do this. \$\endgroup\$ – James Rivera Aug 1 '16 at 21:09
  • \$\begingroup\$ One crude way to measure elapsed time would be to print the results of microtime(). I would use it to measure the time taken by the REST query, the cumulative time taken by SELECT, and the cumulative time to construct the $current_onsites structure. \$\endgroup\$ – 200_success Aug 1 '16 at 21:15
1
\$\begingroup\$

Some thoughts:

  • I honestly don't see how this code "works" as shown. There are significant problems in your code as I will detail below.
  • Don't use mysql_* functions. They have been deprecated in PHP for several years now (since PHP 5.5 in June 2013) and are now totally removed from PHP 7. Consider mysqli or PDO (mysqli being the most similar to mysql in syntax)
  • It is not clear why you have so many loops here. This will slow down your code if you have large result sets you are working with. There doesn't seem to be any reason that you couldn't build your SQL query in that first foreach loop after the curl call.
  • There seems to be a significant mismatch between the variables set in the first foreach loop after the curl call and the foreach loop where you do your DB queries. There are some variables in the first loop that are not used in the second (so why set them at all?), and there are variable references in the second loop that aren't set in the first. So either there is more code here that you are not showing, or there is no way this code actually works.
  • You have very minimal code comments, so it makes it really hard to understand what your intent is in large areas of the code.

Below, I have made some more specific code comments within multi-line comments

<?php
/*
Does your code absolutely need this to run?
If so, this should probably be a require
*/
include 'schedule.php';

/*
After this line you do nothing to check that curl_init() was successful
and continue operationg assuming happy path. What if init fails?
*/
$curl = curl_init();

/*
This could be contributing to your performance problem as you are making these
requests in a loop.  You might consider curl_multi_exec() to make
requests in parallel
*/
foreach ($onsites as $value){

/*
Indent so it is clear this all happens inside above loop
*/
curl_setopt_array($curl, array(
    CURLOPT_URL => "https://portal.-----.com/v4_6_release/apis/3.0/service/tickets?conditions=id%3D$value",
    CURLOPT_RETURNTRANSFER => true,
    CURLOPT_ENCODING => "",
    CURLOPT_MAXREDIRS => 10,
    CURLOPT_TIMEOUT => 30,
    CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1,
    CURLOPT_CUSTOMREQUEST => "GET",
    CURLOPT_HTTPHEADER => array(
        "accept: application/json",
        "authorization: -----",
        "cache-control: no-cache",
        "postman-token: -----",
        "x-cw-usertype: integrator"
    )
));

$response = curl_exec($curl);
$err      = curl_error($curl);

/*
If this is script that is not exposed to users echoing out errors might be OK.
If this is user-facing, you probably want to log errors and give
user-friendly error messaging
*/
if ($err) {
    echo "cURL Error #:" . $err;
} else {
    $response = json_decode($response, true);

/*
There is no way your application is working the way you expect as
you are overwriting most of these variables with each loop iteration
without actually doing anything with them first
*/
    foreach ($response as $ticket) {            
        $ticket_number = $ticket['id'];
        $summary[]       = $ticket['summary'];
        $company[]       = $ticket['company']['name'];
        $site[]          = $ticket['siteName'];
        $addressLine1  = str_replace('# D', '#D', $ticket['addressLine1']);
        $addressLine2  = $ticket['addressLine2'];
        $city          = $ticket['city'];
        $state         = $ticket['stateIdentifier'];
        $zip           = $ticket['zip'];
        $contact[]       = $ticket['contact']['name'];
        $resources     = $ticket['resources'];
        if(empty($addressLine2)) { 
            $full_address[]   = "$addressLine1, $city, $state, $zip, USA";
        } else {
            $full_address[]   = "$addressLine1, $addressLine2, $city, $state, $zip, USA";
        }
    }
}
}
/*
Don't use mysql_* functions
*/
mysql_connect("localhost", "root", "") or die(mysql_error());
mysql_select_db("scheduler") or die(mysql_error());

/*
Are you really trying to do an assignment here or a comparison?
You are actually now doing an assignment which is then evaluated
for it's truthiness.
If you are trying to do a comparison, you need to actually use
== or ===
I am not sure why you would be comparing an array ($onsites) against
a single string value ($ticket_number) though.
*/
if ($ticket_number = $onsites) {
/*
Indent everything inside braces
*/
$current_onsites = array();
$i = 0;

/*
Why all the error suppression here?  That is REALLY bad coding practice.
Again since you are overwriting variables in loop above, there is no way
this is working correctly if you have more than one ticket in response.
Querying in a loop should usually be a red flag. You are usually better
off trying to get all the data you need in a single query if possible,
which it appears to be in this case. You could actually build array
of all values you are trying to get from the database and have a
single query such as:
SELECT company, lat, lng FROM xy
WHERE company IN ('company1', 'company2', ...)
*/
for ($i = 0; $i <= count($ticket_number); $i++) {
    @$current_onsites[$i]['id'] = $ticket_number[$i];
    @$current_onsites[$i]['text'] = $summary[$i];
    @$current_onsites[$i]['location'] = $location[$i];
    @$current_onsites[$i]['company'] = $company[$i];
    @$current_onsites[$i]['site'] = $site[$i];
    @$current_onsites[$i]['location'] = $full_address[$i];
    @$current_onsites[$i]['contact'] = $contact[$i];
    @$current_onsites[$i]['start_date'] = date('Y-m-d H:i:s', strtotime($startDate[$i]));
    @$current_onsites[$i]['end_date'] = date('Y-m-d H:i:s', strtotime($endDate[$i]));
    @$current_onsites[$i]['technician'] = $technician[$i];
    @$current_onsites[$i]['hours'] = $hours[$i];
    @$current_onsites[$i]['status'] = $status[$i];
    @$current_onsites[$i]['done'] = $doneFlag[$i];
    @$current_onsites[$i]['ownerFlag'] = $ownerFlag[$i];
    // Retrieve all the data from the db table
/*
Your LIKE is meaningless here since you have no wildcard.  Just use =
or IN if you take above advice and remove query from the loop.
Don't use SELECT *, it is really bad coding habit.  Just specify
the fields you actually need - in this case 'lat' and 'lng'.
*/
    $result = mysql_query('SELECT * FROM xy WHERE company LIKE "'.@$current_onsites[$i]['company'].'";')
        or die(mysql_error());  
    // store the record of the "example" table into $row
    $row = mysql_fetch_array( $result );    
    @$current_onsites[$i]['lat'] = $row['lat'];
    @$current_onsites[$i]['lng'] = $row['lng'];
}

print "<pre>";
print_r ($current_onsites);
print "</pre>";

$fp = fopen('results.json', 'w');
fwrite($fp, json_encode($current_onsites));
fclose($fp);
}
/*
You should close this resource right after you are done working with it.
*/
curl_close($curl);

?>

If you have any desire to leverage curl_multi_exec, feel free to use this simple REST client I have written (either as is, or as inspiration for your own code) - https://github.com/mikecbrant/php-rest-client

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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