2
\$\begingroup\$

I've spent a week getting a very old script that runs between DB2 and MySQL condensed and functioning but I've been told the last few days on SO that my code is horrible and confusing and shouldn't work (all of which may be true).

The script below runs, it prints all of my arrays accordingly and it preforms the insert I need it to.

Anyway, I'm not asking for someone to solve a specific problem, but please, if you can, just gracefully and guidingly point out the issues in my code so that I can have a quantifiable comparison to make and take the feedback to make this code better and possibly better solve any future problems I have with it.

I'm brand new to working with DB2 and still trying to come into my own in PHP, so I'm just looking for some helpful guidance here.

<?php

//SELECT Order Ids from mysql table to see if status is 'S'
$orderShippedCheck = "
SELECT 
order_id,
order_status
FROM order_status
WHERE order_status = 'S'
";

//get result and store in array
$result = mysqli_query($mysqlConn, $orderShippedCheck);
$order_ids = array();


//loop results to gather order IDs and store them
while ($row = mysqli_fetch_array($result)){
    $order_ids[] = $row['order_id'];
}

print_r($order_ids);   /*This prints the correct order IDs*/

$orderIdsStr = "'" . implode("', '", $order_ids) . "'";
//SELECT FROM DB2 WITH THE ORDER NUMBERS FIRST
$query = "SELECT invnoc as INVOICE,
                 cstnoc AS DEALER,
                 framec AS FRAME,
                 covr1c AS COVER,
                 colr1c AS COLOR ,
                 extd2d AS SHIPDATE,
                 orqtyc AS QUANTITY
          FROM GPORPCFL
          WHERE invnoc IN ({$orderIdsStr})
          GROUP BY invnoc,cstnoc, slsnoc, orqtyc, framec, covr1c,colr1c, extd2d
          ORDER BY invnoc asc";


$Db2ShipRslt = odbc_exec($DB2Conn, $query);
if ( $Db2ShipRslt === false ) {
     exit (odbc_errormsg($DB2Conn));
}


//Process the results
$Db2ShipArr = array();
while($db2ShipRow = odbc_fetch_array($Db2ShipRslt))
{
    //Output the record
    print_r($db2ShipRow);    /*This prints the correct Rows*/
    //Append record to results array
    $Db2ShipArr[] = $db2ShipRow;
}

foreach($Db2ShipArr as $Db2Ship){

    //Check to see if there are any records in jfi_sales.placements for the dealer/sku combo just pulled from DB2
    $dealerSkuCheck = "
    SELECT 
    sku_id,
    dealer_id
    FROM placements_new p
    INNER JOIN skus s
    ON p.sku_id = s.id
    WHERE p.dealer_id = '{$Db2Ship['DEALER']}'
    AND   s.frame = '{$Db2Ship['FRAME']}'
    AND   s.cover1 = '{$Db2Ship['COVER']}'
    AND   s.color1 = '{$Db2Ship['COLOR']}'
    AND   p.order_num = '{$Db2Ship['INVOICE']}'
    ";

    //IF those records exist for the dealer/sku, store them in array
    $existingCheckRslt = mysqli_query($mysqlConn, $dealerSkuCheck);
    $existingRecords = array();

    while ($existingRow = mysqli_fetch_array($existingCheckRslt, MYSQLI_ASSOC)){
        $existingRecords[] = $existingRow;
    }

    $row_cnt = mysqli_num_rows($existingCheckRslt);

    print_r($existingRecords); /*This prints the right records showing that there exists record(s) in placements that have that dealer_id and sku_id*/

        //If the existingRecords array is empty, meaning no records exist for that dealer/sku, then preform the insert
        if($row_cnt == 0){
        //INSERT # of records equal to QUANTITY
            $stmt = $PDO->prepare("
                INSERT IGNORE INTO placements_new (sku_id, group_id, dealer_id, start_date, expire_date, locations, order_num)
                 SELECT 
                 id, 
                 sku_group_id, 
                 :DEALER, 
                 DATE_ADD(DATE_FORMAT(CONVERT(:SHIPDATE, CHAR(20)), '%Y-%m-%d'),INTERVAL 7 DAY) as start_date,
                 DATE_ADD(DATE_FORMAT(CONVERT(:SHIPDATE, CHAR(20)), '%Y-%m-%d'),INTERVAL 127 DAY) as expire_date, 
                 :QUANTITY,
                 :INVOICE  
                 FROM skus s
                 WHERE  s.frame=:FRAME AND s.cover1=:COVER AND s.color1=:COLOR 
            ");


        $PDO->beginTransaction();
        $i = 0;
        while($db2row = odbc_fetch_array($Db2ShipRslt)) {


            if(++$i % 1000 == 0) {
                $PDO->commit();
                $PDO->beginTransaction();
            }
            $stmt->execute($db2row);
        }
        $PDO->commit();
        }
}

//Close Connections
if (mysqli_close($mysqlConn)){
    echo "MySQL Closed";
}

$PDO = null;
$stmt = null;

?>
\$\endgroup\$
  • \$\begingroup\$ So are you getting insertions you don't intend to get? \$\endgroup\$ – Raystafarian Mar 3 '18 at 3:39
  • \$\begingroup\$ Sort of, but they're duplicates. If I truncate the table and run the script, I get 1022 records inserted, which I expect. When I run it again, it should store every record in the expectedRecords array and therefore do no insertion but it inserts every record again \$\endgroup\$ – Tom N. Mar 3 '18 at 3:44
  • 1
    \$\begingroup\$ Well, we can't really help you with that second thing, but we can help you with the code. The part of it not working as expected is off-topic here. \$\endgroup\$ – Raystafarian Mar 3 '18 at 4:25
  • \$\begingroup\$ That's fine, the help with the code was the main thing I'm looking for and hopefully that will help me moving on from here and I can better solve my other issues. Any help or feedback you have is much appreciated \$\endgroup\$ – Tom N. Mar 3 '18 at 4:38
  • 1
    \$\begingroup\$ I hope you don't mind, I edited that part out of the question so you won't get close votes \$\endgroup\$ – Raystafarian Mar 3 '18 at 4:39
2
\$\begingroup\$

I have a few observations from looking at your code. Some of them are related to the PHP code, some of them are related to the SQL. Some of these observations may also help you resolve some of the perceived bugginess of your code.

Let's start with the PHP:

Use only one database abstraction per database type I see that you are using three different database abstractions to access two databases. I would try to bring that down to two: one for your DB2 database, and one for your MySQL database. I am guessing that your code is using both PDO and mysqli against the MySQL database. Choose one and use that. If you need multiple connections to a single database, you can do that with a single abstraction. Use different connection handles or objects to differentiate between your connections, not different database abstractions.

Use functions to structure your code It could be that your code is using multiple database abstractions to keep your MySQL connections separate. A large monolithic block of code can be hard to visualize all at once, particularly if it is doing a lot of stuff. Break things up into functions that just do a single thing, then assemble the functions into a main procedure. For example:

OpenConnections();
$order_ids = GetShippedOrders();
$Db2ShipArr = GetOrdersFromDb2($order_ids);
foreach($Db2ShipArr as $Db2Ship){
  if (GetExistingRows($Db2Ship, $ExistingRows) > 0) {
    foreach($ExistingRows as $ExistingRow) {
      $ExistingRecords[] = $ExistingRow;
    }
  } else {
    InsertRow($Db2Ship);
  }
}
CloseConnections(); 

...

// Add the functions here

This will be easier to understand, and the names of the functions helps document the code.

Let's look at the SQL:

Use prepared statements and parameter markers as much as possible rather than concatenation to build your SQL statements. This will reduce your susceptibility to injection attacks. This is particularly important when concatenating strings together as it is the strings that can contain raw SQL which can change your statement when concatenated together. This includes concatenation by variable replacement as you are doing in several places. This is not as big a deal if you are concatenating in values read from numeric database fields, but you still need to make sure that you don't inadvertently replace numeric values with strings. Since PHP is a dynamically typed language, this is a distinct possibility, and needs to guarded against.

Don't use GROUP BY unless you are aggregating values. There is a group by in your DB2 Query which I can only assume is being used to remove duplicates from the result set. If you are getting duplicates, use DISTINCT instead. It is more semantically correct. When I see a GROUP BY without aggregation terms, I wonder why the GROUP BY is there, but DISTINCT tells me that you only want to see unique rows. Note that your group by could still be providing duplicate rows since there are more terms in the GROUP BY clause than there are in your SELECT clause.

Things that could be causing bugginess:

  1. The GROUP BY
  2. Using the wrong array to provide parameters to your PDO prepared statement because your big chunk of code is unwieldy.
\$\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.