4
\$\begingroup\$

I want to get the number if items with state 4 and 1 from my database for each day between a certain date. Is there a smarter and more performative way than this one?

I am aware that I should use MySQLi and that MySQL is deprecated.

Code explanation:

  • the for loop is to go through everyday between 2 dates
  • $sql_condition is in case the query has a specific item, otherwise empty variable

My code:

for( $thisDay = $start; $thisDay <= $end; $thisDay = $thisDay + 86400){

    $formatedDate = date('Y-m-d', $thisDay);
    // booked houses
    $sql="SELECT id FROM ".T_BOOKINGS." WHERE id_state=1 ".$sql_condition." AND the_date='".$formatedDate."'";
    $res=mysql_query($sql) or die("Error getting states<br>".mysql_Error());

    //reserved houses
    $sql2="SELECT id FROM ".T_BOOKINGS." WHERE id_state=4 ".$sql_condition." AND the_date='".$formatedDate."'";
    $res2=mysql_query($sql2) or die("Error getting states<br>".mysql_Error());

    $bookings['booked'][] = mysql_num_rows($res);
    $bookings['reserved'][] = mysql_num_rows($res2);
    $dates[] = $formatedDate;

}
\$\endgroup\$

1 Answer 1

4
\$\begingroup\$

Three major points:

  • Executing SQL queries in a loop will nearly always cause a performance problem. You want to formulate a query that gives you all the data you want.
  • mysql_query() and similar functions are deprecated; use mysqli or PDO instead.
  • Composing your query by concatenation and interpolation, without escaping, could easily lead to an SQL injection vulnerability. Better yet, use a database interface that supports placeholders for parameters so that you don't have to worry about escaping.

A better solution would go something like this:

function bookings_between($booking_state, $start, $end) {
    $formattedStart = date('Y-m-d', $start);
    $formattedEnd   = date('Y-m-d', $end);

    $query = mysql_query("
        SELECT the_date, COUNT(id)
            FROM ".T_BOOKINGS."
            WHERE id_state=$booking_state
                $sql_condition AND
                the_date BETWEEN '$formattedStart' AND '$formattedEnd'
            GROUP BY the_date;")
        or die ("Error getting states<br>".mysql_error());

    $results = array();
    while ($row = mysql_fetch_array($query, MYSQL_NUM)) {
        $results[$row[0]] = $row[1];
    }
    return $results;
}

$booked = bookings_between(1, $start, $end);
$reserved = bookings_between(4, $start, $end);

The results are not in the same format as your original. This returns the bookings in associative arrays, keyed by the date (as formatted by MySQL). Days that have no bookings will not have an entry. If you need to, you can post-process $booked and $reserved back into a data structure compatible with the original code. The big win, though, is that you execute just two queries instead of two queries per day of the period in question.

\$\endgroup\$
6
  • \$\begingroup\$ This was very interesting! +1 - so you mean that event if I have a loop after this code you suggested, to get back into a array the dates were select did not return the_date, it would anyway be better and faster than my code? \$\endgroup\$
    – Rikard
    Jan 1, 2014 at 12:44
  • 1
    \$\begingroup\$ yes it would, you got it. \$\endgroup\$
    – rolfl
    Jan 1, 2014 at 12:45
  • \$\begingroup\$ Then I say: thak you @rolfl and 200_success, and have a nice 2014 :) \$\endgroup\$
    – Rikard
    Jan 1, 2014 at 12:47
  • \$\begingroup\$ Just implementd your code, had a performance gain of 1,5 second per year between dates! impressive, nice! Wish I could +1 you again :) \$\endgroup\$
    – Rikard
    Jan 1, 2014 at 14:01
  • 1
    \$\begingroup\$ @Rikard You're welcome. Unfortunately, it's too late to be eligible for Best of Code Review 2013. \$\endgroup\$ Jan 1, 2014 at 19:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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