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 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;

}
share|improve this question
add comment

1 Answer 1

up vote 4 down vote accepted

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.

share|improve this answer
    
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? –  Rikard Jan 1 at 12:44
1  
yes it would, you got it. –  rolfl Jan 1 at 12:45
    
Then I say: thak you @rolfl and 200_success, and have a nice 2014 :) –  Rikard Jan 1 at 12:47
    
Just implementd your code, had a performance gain of 1,5 second per year between dates! impressive, nice! Wish I could +1 you again :) –  Rikard Jan 1 at 14:01
1  
@Rikard You're welcome. Unfortunately, it's too late to be eligible for Best of Code Review 2013. –  200_success Jan 1 at 19:27
show 1 more 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.