Take the 2-minute 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 querying all orders from a WordPress database that uses the WooCommerce Bookings plugin. I want to display upcoming bookings, and there is a potential to have every hour within 2015 to be booked; which means that I might query over 8700 orders.

I'm new to PHP and did most of this following code myself. I'm not sure if there is a more efficient way of writing this code. I don't want my code to hinder site performance.

<?php
    function objectToArray($d) {
        if (is_object($d)) {
            $d = get_object_vars($d);
        }

        if (is_array($d)) {
            return array_map(__FUNCTION__, $d);
        } 
        else {
            return $d;
        }
    }

    $results = $wpdb->get_results("SELECT tzYeb_2_posts.* FROM tzYeb_2_posts INNER JOIN tzYeb_2_term_relationships ON (tzYeb_2_posts.ID = tzYeb_2_term_relationships.object_id) WHERE 1=1 AND ( tzYeb_2_term_relationships.term_taxonomy_id IN (11) ) AND tzYeb_2_posts.post_type = 'shop_order' AND ((tzYeb_2_posts.post_status = 'publish')) GROUP BY tzYeb_2_posts.ID ORDER BY tzYeb_2_posts.post_date DESC");

    $time_array = array();
    $fname_array = array();
    $church_array = array();
    $city_array = array();
    $state_array = array();

    $new_array = array();

    foreach ($results as $result) {
        $order_items = $wpdb->get_results("SELECT    order_item_id, order_item_name, order_item_type
                        FROM      {$wpdb->prefix}woocommerce_order_items
                        WHERE     order_id = " . $result->ID . "
                        AND       order_item_type IN ('line_item')
                        ORDER BY  order_item_id");

        foreach ($order_items as $order_item) {
            $item_meta = $wpdb->get_results("
                                        SELECT    *
                                        FROM        {$wpdb->prefix}woocommerce_order_itemmeta
                                        WHERE    order_item_id = " . $order_item->order_item_id . "
                                    ");

            $meta_array = objectToArray($item_meta);

            $timestamp = "";
            $date = "";
            $time = "";
            $fname = "";
            $church = "";
            $city = "";
            $state = "";

            foreach ($meta_array as $data) {
                if ($data['meta_key'] == 'Booking Date')
                    $date = $data['meta_value'];
                if ($data['meta_key'] == 'Booking Time')
                    $time = $data['meta_value'];
                if ($data['meta_key'] == 'First Name - First Name')
                    $fname = $data['meta_value'];
                if ($data['meta_key'] == 'Church Information - Church Name')
                    $church = $data['meta_value'];
                if ($data['meta_key'] == 'Church Information - City')
                    $city = $data['meta_value'];
                if ($data['meta_key'] == 'Church Information - State')
                    $state = $data['meta_value'];
            }

            $timestamp = strtotime($time . ' ' . $date);

            array_push($time_array, $timestamp);
            array_push($fname_array, $fname);
            array_push($church_array, $church);
            array_push($city_array, $city);
            array_push($state_array, $state);
        }
    }

    $arrays = array($fname_array, $church_array, $city_array, $state_array);

    foreach ($time_array as $index => $key) {
        $t = array();
        foreach ($arrays as $array) {
            $t[] = $array[$index];
        }
        $up[$key] = $t;
    }

    ksort($up);

    $i = 0;
    foreach ($up as $tsort => $tval) {
        if ($tsort > strtotime('now')) {
            ?>

            <div class="wc-upcoming-booking">
                <div class="wc-upcoming-time">
                    <span class="upcoming-hour"><?php echo date('g:i A', $tsort); ?></span>
                    <span class="upcoming-date"><?php echo date('F j', $tsort); ?></span>
                </div>
                <div class="wc-upcoming-details">
                    <?php
                    if ($tval[0]) {
                        echo $tval[0];
                    }
                    ?>
                    <?php
                    if ($tval[1]) {
                        echo ', ' . $tval[1];
                    }
                    ?>
                    <?php
                    if ($tval[2]) {
                        echo ', ' . $tval[2];
                    }
                    ?>
                    <?php
                    if ($tval[3]) {
                        echo ', ' . $tval[3];
                    }
                    ?>
                </div>
            </div>

            <?php
            $i++;
            if ($i >= 10)
                break;
        }
    }
?>
share|improve this question

3 Answers 3

The biggest performance issue is probably here:

foreach( $order_items as $order_item ) {
   $item_meta = $wpdb->get_results("
       SELECT  *
       FROM        {$wpdb->prefix}woocommerce_order_itemmeta
       WHERE   order_item_id = " . $order_item->order_item_id . "
   ");
   // ...
}

If you have 1000 order items, you will run a 1000 queries. You could change this to a single query (with a different WHERE condition) to return 1000 records, that will make a big difference in performance.

Another important thing here is to avoid SELECT * queries. It's better to name explicitly the columns you need so you don't select stuff you will throw away anyway. It will also make your implementation somewhat more robust, because the columns you need will always appear in the same expected order, even if you change the schema and reorder the columns. As a general rule of thumb, you should never use SELECT * queries.

Another thing that can improve performance is using prepared statements. Instead of queries like this:

"SELECT x FROM sometable WHERE id = " . $the_id . "

you should use something like this:

$wpdb->query($wpdb->prepare("SELECT x FROM sometable WHERE id = %d", $the_id))

This form protects you from SQL injection attacks because the placeholders will be checked for type, and prepared statements can be compiled and reused by the database.

Finally, there are some coding style issues: the original post had too wide indentation on the left. You also have a very long SQL statements on one line, which is really hard to read, as it forces me to scroll to the right. It's good to make code easy to read for others.

share|improve this answer
    
Thank you for your comments. I updated my code based on your suggestions and I also found a way to remove most of the foreach statements and query what I need from only 1 query. How do I show my updated code in this forum? –  katart Jul 16 at 13:35
    
Post a new question with your revised solution. Mention about the suggestions you decided not to do, and why. Link back to this question, for reference. I think that's about it. Feel free to ping @Jamal or others on the 2nd monitor channel chat.stackexchange.com/rooms/8595/the-2nd-monitor –  janos Jul 16 at 14:27

Yes, this code does look like a performance disaster! I will try to review what I can, but without proper profiling and lack of data structures and code context, it's nearly impossible for someone besides you to streamline this code!

I'll work my way from top to bottom, although there are several things that are screaming to be reviewed!

I cleaned up your first function for you. The name was misleading, because you didn't have to pass just an object, a type array could be passed too.

function makeArray($input) {
    if (is_object($input)) {
        $input = get_object_vars($input);
    }
    if (is_array($input)) {
        return array_map(__FUNCTION__, $input);
    }
    return $input;
}

Alright, now you have a query:

SELECT tzYeb_2_posts.* 
    FROM tzYeb_2_posts 
        INNER JOIN tzYeb_2_term_relationships 
        ON (tzYeb_2_posts.ID = tzYeb_2_term_relationships.object_id)
            WHERE 1=1 AND ( tzYeb_2_term_relationships.term_taxonomy_id IN (11) ) AND tzYeb_2_posts.post_type = 'shop_order' AND ((tzYeb_2_posts.post_status = 'publish')) 
GROUP BY tzYeb_2_posts.ID 
ORDER BY tzYeb_2_posts.post_date DESC

I'm no database professional, but this look disastrous. I notice you check that 1=1, and I don't see the purpose in that. You also have one to many parentheses around your last where condition. I highly recommend you put this query in a stored procedure, I know that could improve performance and clean up your PHP. That actually can be applied to all your queries, stored procedures would really benefit.

Just past your third nested foreach, I see some code such as:

array_push($time_array, $timestamp);

I suggest you use square brackets to improve performance:

$time_array[] = $timestamp;

In this section:

if ($data['meta_key'] == 'Booking Date')
    $date = $data['meta_value'];

I recommend you give your code braces, as that clears things up, and then make this an ifelse. Making it an ifelse will help your code execute faster.

Some of your variables need a new name. What in the world is $arrays? Give the names some meaning so we know what you're talking about!

In that last foreach, I recommend you don't mix your markup and PHP like that. Perhaps use a function such as sprintf so that it's easier to read and is cleaner. As for those conditionals, you could just replace them with:

echo implode(', ',  $tval);

Overall, you're greatest setback in performance has got to be the multiple loops and nested loops. If anything, those are the things you need to reduce. I suggets you learn more about the different array functions and the capabilities or an SQL query, and try and refactor them then.

share|improve this answer

I'm not familiar with the Woocommerce plugin and couldn't have tested this code, so don't delete yours. Chceck if it works and gives you expected result. I can't tell if you really use the Woocommerce data structure properly, but using the woocommerce_order_itemmeta table in this context scares the hell out of me and if there's no better access to ordered items then you're the last one to blame for unefficient code.

The tweaks are based on some ground rules of querying database (mentioned in other answers as well):

  1. Avoid queries inside loops
  2. Ask for as accurate result as you can (don't fetch sth you won't use)

First one could be achieved, but second needs some hack'ish and not fully efficient workarounds, because ability to limit and sort results is limited by given data structure (see comments within code).

    // Find item ids with future booking date to ease next (slow) query.
    // If string format is known and consistent think of filtering with mysql STR_TO_DATE() function.
    // I will use php strtotime() to filter them instead (might be big chunk of data though).
    $items = $wpdb->get_results("
        SELECT m1.order_item_id AS id, m1.meta_value AS date, m2.meta_value AS time
        FROM {$wpdb->prefix}woocommerce_order_itemmeta m1
        INNER JOIN {$wpdb->prefix}woocommerce_order_itemmeta m2 ON m2.order_item_id = m1.order_item_id
        WHERE m1.meta_key = 'Booking Date'
          AND m2.meta_key = 'Booking Time'
        ORDER BY m1.order_item_id ASC
    ");

    $now = time();
    $data = array();
    foreach ($items as $item) {
        $timestamp = strtotime($item->date . ' ' . $item->time);
        if ($timestamp > $now) {
            // $timestamp is used only for sorting and will be unset later,
            // may be used instead 'date' and 'time' fields though.
            $data[$item->id]['timestamp'] = $timestamp;
        }
    }

    // Initially sorted array with ids as key.
    // Other data will be inserted here from next query
    // thus the order will be maintained.
    asort($data);

    // list of ids that match future timestamp
    // can't simply LIMIT to 10 with array_slice() because upcoming conditions could filter them out
    // higher limit also uncertain, but maybe worth a try (loss vs. propability) 
    $item_list = implode(', ', array_keys($data));

    // helper array for fast check and key assign
    $meta_keys = array(
        'Booking Date' => 'date',
        'Booking Time' => 'time',
        'First Name - First Name' => 'fname',
        'Church Information - Church Name' => 'church',
        'Church Information - City' => 'city',
        'Church Information - State' => 'state'
    );

    // Slow query, but only one - nested loop queries are killers.
    $item_meta = $wpdb->get_results("
        SELECT m.order_item_id, m.meta_key, m.meta_value
        FROM {$wpdb->prefix}woocommerce_order_itemmeta m
        INNER JOIN {$wpdb->prefix}woocommerce_order_items i ON m.order_item_id = i.order_item_id
        INNER JOIN {$wpdb->prefix}posts p ON p.ID = i.order_id
        INNER JOIN {$wpdb->prefix}term_relationships r ON r.object_id = p.ID
        WHERE r.term_taxonomy_id = 11
          AND p.post_type = 'shop_order'
          AND p.post_status = 'publish'
          AND m.order_item_id IN ({$item_list})
    ");

    $num = 0; //found items counter   
    foreach ($item_meta as $item) {
        if (!isset($meta_keys[$item->meta_key])) { continue; }

        $id  = $item->order_item_id;
        $key = $meta_keys[$item->meta_key];

        if (!isset($data[$id]['num']) {
            $data[$id]['num'] = $num++;
        }   
        $data[$id][$key] = $item->meta_value;
    }

    $count = 0;
    //$page (int) page number (display last page with results if too high)
    $start = ($num - 11 < $page * 10) ? $num - 11 : $page * 10;
    if ($start < 0) { $start = 0; }
    foreach ($data as $item) {
        // skip results not found by second query
        if (!isset($item['time']) || $item['num'] < $start) { continue; }

        $time = $item['time'];
        $date = $item['date'];
        unset($item['time'], $item['date'], $item['timestamp']);
        $details = implode(',<br>', $item);
?>
    <div class="wc-upcoming-booking">
        <div class="wc-upcoming-time">
            <span class="upcoming-hour"><?php echo $time; ?></span>
            <span class="upcoming-date"><?php echo $date; ?></span>
        </div>
        <div class="wc-upcoming-details">
            <?php echo $details; ?>
        </div>
    </div>
<?php
        $count++;
        if ($count == 10) { break; }

    }

Note: I assume that sorting by Booking Time/Date may differ from sorting by order_item_id. (Further optimization possible if assumption is wrong).

share|improve this answer
    
Interesting solution, 1+ for the effort! If you want, join some other brilliant Code Reviewers in the 2nd Monitor :) –  Alex L Jul 10 at 23:22
    
Welcome to Code Review. Your answer looks like it could be promising. However, it lacks an explanation of what you changed, and why. –  200_success Jul 11 at 1:32
    
@200_success - Right. I'll fix the code and explain some details when I get home. –  shudder Jul 11 at 9:02
    
@shudder This works great. I assume that both queries still have the potential of grabbing over 8700 orders. Correct? –  katart Jul 17 at 17:59
    
@katart This is best what I could come up with. It would work, but I think that for such data load plugin extension/refactoring is neccessary. The bottleneck at the moment is lack of booking timestamp/datetime field that can be recognised by db engine (presumably in woocommerce_order_items). I believe there are some extensions/forks available that allow woocommerce to deal with booking functionality. –  shudder Jul 17 at 18:59

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.