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;
}
1 Answer 1
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; usemysqli
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.
-
\$\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– Rikard2014年01月01日 12:44:33 +00:00Commented Jan 1, 2014 at 12:44 -
1\$\begingroup\$ yes it would, you got it. \$\endgroup\$rolfl– rolfl2014年01月01日 12:45:20 +00:00Commented Jan 1, 2014 at 12:45
-
\$\begingroup\$ Then I say: thak you @rolfl and 200_success, and have a nice 2014 :) \$\endgroup\$Rikard– Rikard2014年01月01日 12:47:28 +00:00Commented 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– Rikard2014年01月01日 14:01:12 +00:00Commented 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\$200_success– 200_success2014年01月01日 19:27:46 +00:00Commented Jan 1, 2014 at 19:27