Optimise advice for many loops in php?

Question

I have been working on this code piecing it together here and there as it came and works, and the result is VERY MESSY!

I just need some advice what i should do to reduce the amount of loops or perhaps are there any loops you see that shouldn't be needed?

any advice to the below code is appreciated.

if (isset($_POST['refresh-history'])):
  $order_id = $_POST['id'];
  $order = $database->get_results('SELECT * FROM `orders` WHERE `order_id`='.$order_id);
  $matches = $database->get_results('SELECT `match_id` FROM `matches` WHERE `order_id`='.$order_id);
  foreach ($order as $o):
    $loluser = $o->loluser;
    $region = $o->region;
    $date_created = $o->date_created;
    $date_completed = $o->date_completed;
  endforeach;
  $api->setRegion($region);
  $matchlistapi = $api->matchlist();
  $matchapi = $api->match();
  $matchlist = $matchlistapi->matchlist($loluser, "RANKED_SOLO_5x5", "SEASON2015", null, null, null, $date_created, $date_completed);
  if ($matchlist->totalGames !== 0):
    foreach ($matchlist as $key):
      $gameIds[] = $key->matchId;
    endforeach;
    $arr_matches = object2array($matches);
    foreach ($arr_matches as $id) {
      $dbMatches[] = (int)$id->match_id;
    }
    $new_array = array_diff($gameIds, $dbMatches);

    foreach ($new_array as $matchId):
      $games[] = $matchapi->match($matchId, false);
    endforeach;
    foreach ($games as $game):
      // Store Games in DB;
    endforeach;
    $_SESSION['api_success'] = "Success: Games Synced to Order.";
  else:
    $_SESSION['error_msg'] = "Error 23: Unable to Find Games.";
  endif;
endif;

To be Clear I DO NOT NEED AN ANSWER! Just a PUSH in the right direction, i can go from there. :)

Answer 1

You do not need to reduce the amount of loop or if else statement. The key to making your code more readable is to group them into logical group. Consider something like this.

function getMatchDetail($orderId) {
    $matchDetail = array();
    $order_id = $_POST['id'];
    $order = $database->get_results('SELECT * FROM `orders` WHERE `order_id`='.$order_id);
    $matches = $database->get_results('SELECT `match_id` FROM `matches` WHERE `order_id`='.$order_id);
    // I do not know what do get_results will return. So I am going to use my best guess
    $matchDetail[$match]["loluser"] = $order["loluser"];
    $matchDetail[$match]["region"] = $order["region"];
    $matchDetail[$match]["date_created"] = $order["date_created"];
    $matchDetail[$match]["date_completed"] = $order["date_completed"];
    return $matchDetail;
}

function getMatchListFromAPI($matchDetail) {
    // Again, just giving you a general direction;
    $api = new API();
    $api->setRegion($matchDetail["region"]);
    $matchList = $api->getList($matchDetail["loluser"], $matchDetail["date_created"], $matchDetail["date_completed"]);
    return $matchList;
}

function doStuffWithMatchList($matchList) {
    foreach ($matchlist as $key):
        $gameIds[] = $key->matchId;
    endforeach;
    $arr_matches = object2array($matches);
    foreach ($arr_matches as $id) {
        $dbMatches[] = (int)$id->match_id;
    }
    $new_array = array_diff($gameIds, $dbMatches);

    foreach ($new_array as $matchId):
        $games[] = $matchapi->match($matchId, false);
    endforeach;
    foreach ($games as $game):
        // Store Games in DB;
    endforeach;
}

if (isset($_POST['refresh-history'])):
    $matchDetail = getMatchDetail($_POST["id"]);
    $matchList = getMatchListFromAPI($matchDetail);
    if ($matchlist["totalGames"] !== 0):
        doStuffWithMatchList($matchList);
        $_SESSION['api_success'] = "Success: Games Synced to Order.";
    else:
        $_SESSION['error_msg'] = "Error 23: Unable to Find Games.";
    endif;
endif;

Now it is very clear that what your program does.

Also commenting your code will help

Answer 2

This code is doing the following:

• Get the information in the DB corresponding to the current order:

  'SELECT * FROM `orders` WHERE `order_id` = '.$order_id);
  

Here the problem is that order_id should be your primary key so no loop is needed.

• Get the match lists matching the current order with the API

    $matchlistapi->matchlist(...)
  

• Get the match lists matching the current order within the Database

• Store games in the DB which are not already present

All that is useless. What is important is to store the match in a unique way.

Simply define a multi column unique key on your matches tables

ALTER TABLE matches ADD UNIQUE (order_id, ..., ...)

and use INSERT IGNORE INTO matches when you will insert.

So the elements which already are in the matches table won't be overwritten.

Your code will be something similar to:

$result = $database->get_results('SELECT loluser, region, date_created, date_completed
             FROM `orders` WHERE `order_id` = '.$order_id);

list($loluser, $region, $date_created, $date_completed)=$result[0];

$matchlistapi = $api->matchlist();
$matchlist = $matchlistapi->matchlist($loluser, "RANKED_SOLO_5x5",
            "SEASON2015", null, null, null, $date_created, $date_completed);

$matchapi = $api->match();
foreach ($matchlist as $m) {
   $match=$matchapi->match($m->match_id, false);
   $stmt=$database->prepare("INSERT IGNORE INTO matches VALUES(?, ?, ?)", ...);
   $database->execute($stmt);
}

Note: Of course you have to do care about potential SQL injection in your first request

Answer 3

Explanation

You can replace the following code

foreach ($order as $o):
    $loluser = $o->loluser;
    $region = $o->region;
    $date_created = $o->date_created;
    $date_completed = $o->date_completed;
endforeach;

with

//get the last order.
$order = end($orders);

//set the information.
$loluser = ($order === false) ? '' : $order->loluser;
$region = ($order === false) ? '' : $order->region;
$date_created = ($order === false) ? '' : $order->date_created;
$date_completed = ($order === false) ? '' : $order->date_completed;

There are solved two problems. The first thing is, that your foreach run through all orders and write the properties of every order to the variables. At the end only the properties of the last order will be set on variables. The second thing is, that if no order is available the variables are not set on the following script.

On the sync part i could remove some variables which are not needed because they only stores an array for the next loop. But you can use the result of the function itself on the loop if you don't check the result of the functions before.

An example. You can replace the following code

$new_array = array_diff($gameIds, $dbMatches);

foreach ($new_array as $matchId):
    $games[] = $matchapi->match($matchId, false);
endforeach;

can be replaced with

foreach (array_diff($gameIds, $dbMatches) as $matchId):
    $games[] = $matchapi->match($matchId, false);
endforeach;

The Result

Here you can find the full code of your script with some optimizations:

<?php
if (isset($_POST['refresh-history'])) {
    $order_id = $_POST['id'];
    $orders = $database->get_results('SELECT * FROM `orders` WHERE `order_id` = '.$order_id);
    $matches = $database->get_results('SELECT `match_id` FROM `matches` WHERE `order_id` = '.$order_id);

    //get the last order.
    $order = end($orders);

    //set the information.
    $loluser = ($order === false) ? '' : $order->loluser;
    $region = ($order === false) ? '' : $order->region;
    $date_created = ($order === false) ? '' : $order->date_created;
    $date_completed = ($order === false) ? '' : $order->date_completed;

    $api->setRegion($region);
    $matchlistapi = $api->matchlist();
    $matchapi = $api->match();
    $matchlist = $matchlistapi->matchlist($loluser, "RANKED_SOLO_5x5", "SEASON2015", null, null, null, $date_created, $date_completed);

    //check if a game is available.
    if ($matchlist->totalGames > 0) {

        //initialize the vars.
        $matchIds = array();
        $matchIdsDB = array();

        //collect all match ids from api.
        foreach ($matchlist as $match) {
            $matchIds[] = (int) $match->matchId;
        }

        //collect all match ids from database.
        foreach (object2array($matches) as $match) {
            $matchIdsDB[] = (int) $match->match_id;
        }

        //run through all missing matches.
        foreach (array_diff($matchIds, $matchIdsDB) as $match) {
             $game = $matchapi->match($match, false);

             //store game in database or create a big query to create all in one.
        }

        $_SESSION['api_success'] = "Success: Games Synced to Order.";
    } else {
        $_SESSION['error_msg'] = "Error 23: Unable to Find Games.";
    }
}

Hope it helps!

Answer 4

 foreach ($order as $o):
    $loluser = $o->loluser;
    $region = $o->region;
    $date_created = $o->date_created;
    $date_completed = $o->date_completed;
  endforeach;

This loop always return the last value of the array, so what's the point to use loop here?

For other loop, the purpose now is getting a value from array element?. You can wrap the loop in generic way, and return the new array. WordPress has a function which you can reuse https://codex.wordpress.org/Function_Reference/wp_list_pluck