Take the tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am updating some old reports that are using mysql_ statements and trying to use the MySQL PDO stuff. I was told that PDO is far better since I was running into runtime issues with some of my reports. My old report took 91 seconds to run, and my PDO version takes 106 seconds. While 15 seconds may not seem like a big deal, this report is dealing with 1 week's worth of data, and other reports deal with a month up to a year. Additionally, my $cart_total doesn't seem to work in the PDO version.

I would appreciate any help optimizing my queries (though I think they are pretty solid), and my PHP/PDO code.

<?php
    $start_time = time();
    require_once('db_configuration.php');

    $db = new PDO('mysql:host=' . $db_host . ';dbname=' . $db_name . ';charset=utf8', $db_username, $db_password, array(PDO::ATTR_EMULATE_PREPARES => false, 
                                                                                                PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION));

    $key_query_raw = "SELECT configuration_key, configuration_value FROM configuration WHERE configuration_group_id = 6501 AND configuration_key IN('RCS_BASE_DAYS', 'RCS_EMAIL_TTL', 'RCS_SKIP_DAYS') ORDER BY configuration_key ASC;";
    try { $key_query = $db->query($key_query_raw); }
        catch(PDOException $ex) { echo "An Error occured! <br><br>" . $ex; }

    while($key = $key_query->fetch(PDO::FETCH_ASSOC)) 
    {
        if ($key['configuration_key'] == 'RCS_BASE_DAYS') { $base_days = $key['configuration_value']; }
        elseif ($key['configuration_key'] == 'RCS_EMAIL_TTL') { $ttl_days = $key['configuration_value']; }
        elseif ($key['configuration_key'] == 'RCS_SKIP_DAYS') { $skip_days = $key['configuration_value']; }
    }

    $key_query->closeCursor();

    $skip_date =  date('Ymd',strtotime('-'.$skip_days.' day',time()));
    $base_date =  date('Ymd',strtotime('-'.$base_days.' day',time()));
    $ttl_date =  date('Ymd',strtotime('-'.$ttl_days.' day',time()));
?>
<html>
    <style type="text/css">
        .row {
            padding-left:5px;
            padding-right:5px;
            border-style:solid;
            border-color:black;
            border-width:1px;
            border-width:0 0 1 0;
        }

        .header {
            background-color:#C8C8C8;
            text-align:left;
            font-weight:bold;
            padding-left:5px;
            padding-right:5px;
            border-style:solid;
            border-color:black;
            border-width:0 0 3 0;
        }
    </style>

    <head>
        <title>Recover Cart Sales Test</title>
        <link rel="stylesheet" type="text/css" href="reports.css" />
        <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    </head>

    <body>
        <table style="border-collapse:collapse;" width=100%>
            <tr>
                <td class="header">Contacted</td>
                <td class="header">Date</td>
                <td class="header">Customer Name</td>
                <td class="header" colspan=2>Email</td>
                <td class="header">Phone</td>
                <td class="header">&nbsp;</td>
            </tr>

            <tr>
                <td class="header">&nbsp;</td>
                <td class="header">Item</td>
                <td class="header" colspan=2>Description</td>
                <td class="header" width=20>Qty</td>
                <td class="header" width=20>Price</td>
                <td class="header" width=20>Total</td>
            </tr>

<?


    $customer_query_raw = "SELECT DISTINCT cb.customers_id, cb.customers_basket_date_added, c.customers_firstname, c.customers_lastname, c.customers_email_address, c.customers_telephone, sc.datemodified AS last_contacted
                             FROM customers_basket cb
                             INNER JOIN customers c ON c.customers_id = cb.customers_id
                             LEFT JOIN scart sc ON cb.customers_id = sc.customers_id
                             WHERE cb.customers_basket_date_added < " . $skip_date . " 
                                AND cb.customers_basket_date_added > " . $base_date . " 
                                AND cb.customers_id NOT IN (SELECT sc.customers_id FROM scart sc WHERE sc.datemodified > " . $ttl_date . ")
                             ORDER BY cb.customers_basket_date_added DESC;";

    try { $customer_query = $db->query($customer_query_raw); }
        catch(PDOException $ex) { echo "An Error occured! <br><br>" . $ex; }

    $customer_row_count = $customer_query->rowCount();

    while($customer = $customer_query->fetch(PDO::FETCH_ASSOC)) 
    {
        $product_query_raw = "SELECT cb.customers_id, cb.products_id, p.products_model, pd.products_name, cb.customers_basket_quantity, p.products_price, (p.products_price * cb.customers_basket_quantity) AS product_total
                                FROM customers_basket cb, products p, products_description pd
                                WHERE cb.customers_id = " . $customer['customers_id'] . " 
                                    AND cb.products_id = pd.products_id
                                    AND p.products_id = pd.products_id";
        try { $product_query = $db->query($product_query_raw); }
        catch(PDOException $ex) { echo "An Error occured! <br><br>" . $ex; }

        $cart_total_query_raw = "SELECT SUM( p.products_price * cb.customers_basket_quantity ) AS cart_total
                                    FROM customers_basket cb, products p
                                    WHERE cb.customers_id = " . $customer['customers_id'] . " 
                                    AND cb.products_id = p.products_id;";
        try { $cart_total_query = $db->query($cart_total_query_raw); }
        catch(PDOException $ex) { echo "An Error occured! <br><br>" . $ex; }

        $result = $cart_total_query->fetchAll(PDO::FETCH_ASSOC);
        $cart_total = $result['cart_total'];
        $cart_total_query->closeCursor();

        $last_contacted = ($customer['last_contacted'] < $ttl_date || $customer['last_contacted'] == NULL) ? 'Uncontacted' : date('Y-m-d', strtotime($customer['last_contacted']));
?>
        <tr>
            <td class="row"><?= $last_contacted; ?></td>
            <td class="row"><?= date('Y-m-d', strtotime($customer['customers_basket_date_added'])); ?></td>
            <td class="row"><?= $customer['customers_firstname'] . ' ' . $customer['customers_lastname']; ?></td>
            <td class="row" colspan=2><?= $customer['customers_email_address']; ?></td>
            <td class="row"><?= $customer['customers_telephone']; ?></td>
            <td class="row">&nbsp;</td>
        </tr>       
<?
        while($product = $product_query->fetch(PDO::FETCH_ASSOC))
        {
?>
            <tr>
                <td>&nbsp;</td>
                <td class="row"><?= $product['products_model']; ?></td>
                <td class="row" colspan=2><?= $product['products_name']; ?></td>
                <td class="row" width=20><?= $product['customers_basket_quantity']; ?>x </td>
                <td class="row" width=20><?= $product['products_price']; ?></td>
                <td class="row" width=20><?= $product['product_total']; ?></td>
            </tr>
<?
        }
?>
            <tr>
                <td colspan=7 style="font-weight:bold; text-align:right;">Cart Total: <?= $cart_total; ?></td>
            </tr>

            <tr>
                <td>&nbsp;</td>
            </tr>

<?
            $product_query->closeCursor();
    } // End While

    $customer_query->closeCursor(); 
    $db = NULL;
?>
        </table>
        <br><br>
<?
    $end_time = time();

    echo "Number of Records: " . $customer_row_count . "<br>";
    echo "Start: " . $start_time . "<br>";
    echo "End: " . $end_time . "<br>";
    echo "Time Elapsed: " . ($end_time - $start_time);
?>  
    </body>
</html>
share|improve this question
 
Fixed my non-working $cart_total by using fetch vs fetchALL (not sure why I had fetchAll). –  Dizzy49 Sep 18 at 2:19
 
if speed is what you're after, forget about PDO, and go for mysqli_*. It's the fastest of the two. And don't forget the classic DNS bottleneck, in case you're passing the host-name as a string, and not IP... And don't, for the love of God do this: catch(PDOException $ex) { echo "An Error occured! <br><br>" . $ex; }. Not only is $ex an object, you're not stopping the rest of the script, it'll still continue to run as though the error never occurred. That's just plain wrong –  Elias Van Ootegem Sep 18 at 8:56
 
I also noticed you're using the short-tag quite a few times. Now this is probably down to the code being legacy and all that, but when mixing PHP in with markup the <? tag isn't a great idea. It never is, really, because <?xml<--: having short-tags enabled, enables PHP parsing on what, essentially, is XML –  Elias Van Ootegem Sep 18 at 11:07
 
@Elias Thank you for your comments, they were very useful. What would be a better approach for the error? For now, I want it to print the error on-screen for debugging. Once complete, it will write to a logfile (which I know how to do). I should probably use $ex->getMessage() vs just $ex though... –  Dizzy49 Sep 19 at 16:21
add comment

1 Answer

up vote 1 down vote accepted

if i was you, i will try to add somekind of index to your MySQL database for this fields:

cb.customers_basket_date_added
cb.customers_id

when this is done i will changes your SQL to a kind of this SQL

SELECT DISTINCT 
            cb.customers_id, 
            cb.customers_basket_date_added, 
            c.customers_firstname, 
            c.customers_lastname, 
            c.customers_email_address, 
            c.customers_telephone, 
            sc.datemodified AS last_contacted

            FROM 
                customers_basket cb INNER JOIN customers c ON c.customers_id = cb.customers_id
                LEFT JOIN scart sc ON cb.customers_id = sc.customers_id

            WHERE 
                ( cb.customers_basket_date_added BETWEEN '{start-date}' AND '{end-date}' ) AND
                NOT EXISTS (
                    SELECT  
                        sc.customers_id 

                    FROM 
                        scart sc 

                    WHERE
                        sc.datemodified > '{date-modified}' AND
                        sc.{customerId-field} = cb.customers_id )

            ORDER BY 
                cb.customers_basket_date_added DESC

i have change your NOT IN function and changes it to NOT EXISTS, and change a lot more.

why are you using distinct and not group by function?

when this SQL perfome fast, we can go to next step the SQL inside your loop, is it posible you can make you SQL dump so i can download it to help you more? i think you have a big mistake in your 3 SQL, maby its can be done width only 1 SQL select and not 1 select * 2 select each loop run, this mean if you got 90 rows out you make 181 selects, its take a big perfome from you MySQL and i pretty sure you can do it on only one select, :)

share|improve this answer
 
Thank you for your response, it was very useful. While I think I could ultimately combine the 'customer' query and the 'product' query, I could not figure out how to combine the 'cart total' query. Which is faster, performing a query that would return the 1 record with the SUM, or to add the products as I loop through them via PHP $cart_total =+ $cart_total + $product['product_total'] –  Dizzy49 Sep 19 at 16:26
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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