Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

SQL

I would write the query as

SELECT t.`value` AS time_of_day, d.`sensor_id`
 FROM `times` t
 LEFT OUTER JOIN `data` d
 ON t.`mysql_hour_minute` = EXTRACT(HOUR_MINUTE FROM d.`datetime`)
 WHERE
 d.`datetime` BETWEEN '2014-11-05' AND '2014-11-05 23:59:59'
 AND d.`sensor_id` IN (1, 2, 3, 4, 5, 999)
 GROUP BY t.`value`, d.`sensor_id`
 HAVING COUNT(t.`value`) > 0
 ORDER BY d.`sensor_id` ASC, t.`value` ASC;

Note the following changes:

  • Changed RIGHT JOIN to LEFT OUTER JOIN, as left joins are more common and natural. Also, it matches the order in which the selected columns are presented.
  • Use the appropriate datetime function to avoid the LIKE operator for the join condition. Note that you'll have to add a column (which I've called mysql_hour_minute) to the times table, with integer values 0, 1, 2, ..., 59, 100, 101, ..., 159, 200, ..., 2359.
  • To accomplish WHERE d.datetime LIKE '...', the database would need to stringify every item. Stringifying is expensive in the first place; having to do it for every row is even worse. If you use a BETWEEN operator, then it can take advantage of an index take advantage of an index. (And I hope you do have an index on the datetime column.)
  • If all you need to know is whether the sensors collected any data in each time slot, you could just put a HAVING condition on the query instead of requesting the count.

In addition, based on the fact that you accidentally left a $ in the example date, I suspect that you are composing the query by string interpolation, rather than by parameter substitution. You might have an SQL injection vulnerability.

PHP

A more idiomatic way to repeat a loop 24 times is:

for ($i = 0; $i < 24; $i++) { ... }

as it includes the number 24. Starting from 0 but using the <= comparison is a bit odd.

The loops would be better written as

for ($h = 0; $h < 24; $h++) {
 for ($m = 0; $m < 60; $m++) {
 $time = sprintf('%02d:%02d', $h, $m);
 ...
 }
}

SQL

I would write the query as

SELECT t.`value` AS time_of_day, d.`sensor_id`
 FROM `times` t
 LEFT OUTER JOIN `data` d
 ON t.`mysql_hour_minute` = EXTRACT(HOUR_MINUTE FROM d.`datetime`)
 WHERE
 d.`datetime` BETWEEN '2014-11-05' AND '2014-11-05 23:59:59'
 AND d.`sensor_id` IN (1, 2, 3, 4, 5, 999)
 GROUP BY t.`value`, d.`sensor_id`
 HAVING COUNT(t.`value`) > 0
 ORDER BY d.`sensor_id` ASC, t.`value` ASC;

Note the following changes:

  • Changed RIGHT JOIN to LEFT OUTER JOIN, as left joins are more common and natural. Also, it matches the order in which the selected columns are presented.
  • Use the appropriate datetime function to avoid the LIKE operator for the join condition. Note that you'll have to add a column (which I've called mysql_hour_minute) to the times table, with integer values 0, 1, 2, ..., 59, 100, 101, ..., 159, 200, ..., 2359.
  • To accomplish WHERE d.datetime LIKE '...', the database would need to stringify every item. Stringifying is expensive in the first place; having to do it for every row is even worse. If you use a BETWEEN operator, then it can take advantage of an index. (And I hope you do have an index on the datetime column.)
  • If all you need to know is whether the sensors collected any data in each time slot, you could just put a HAVING condition on the query instead of requesting the count.

In addition, based on the fact that you accidentally left a $ in the example date, I suspect that you are composing the query by string interpolation, rather than by parameter substitution. You might have an SQL injection vulnerability.

PHP

A more idiomatic way to repeat a loop 24 times is:

for ($i = 0; $i < 24; $i++) { ... }

as it includes the number 24. Starting from 0 but using the <= comparison is a bit odd.

The loops would be better written as

for ($h = 0; $h < 24; $h++) {
 for ($m = 0; $m < 60; $m++) {
 $time = sprintf('%02d:%02d', $h, $m);
 ...
 }
}

SQL

I would write the query as

SELECT t.`value` AS time_of_day, d.`sensor_id`
 FROM `times` t
 LEFT OUTER JOIN `data` d
 ON t.`mysql_hour_minute` = EXTRACT(HOUR_MINUTE FROM d.`datetime`)
 WHERE
 d.`datetime` BETWEEN '2014-11-05' AND '2014-11-05 23:59:59'
 AND d.`sensor_id` IN (1, 2, 3, 4, 5, 999)
 GROUP BY t.`value`, d.`sensor_id`
 HAVING COUNT(t.`value`) > 0
 ORDER BY d.`sensor_id` ASC, t.`value` ASC;

Note the following changes:

  • Changed RIGHT JOIN to LEFT OUTER JOIN, as left joins are more common and natural. Also, it matches the order in which the selected columns are presented.
  • Use the appropriate datetime function to avoid the LIKE operator for the join condition. Note that you'll have to add a column (which I've called mysql_hour_minute) to the times table, with integer values 0, 1, 2, ..., 59, 100, 101, ..., 159, 200, ..., 2359.
  • To accomplish WHERE d.datetime LIKE '...', the database would need to stringify every item. Stringifying is expensive in the first place; having to do it for every row is even worse. If you use a BETWEEN operator, then it can take advantage of an index. (And I hope you do have an index on the datetime column.)
  • If all you need to know is whether the sensors collected any data in each time slot, you could just put a HAVING condition on the query instead of requesting the count.

In addition, based on the fact that you accidentally left a $ in the example date, I suspect that you are composing the query by string interpolation, rather than by parameter substitution. You might have an SQL injection vulnerability.

PHP

A more idiomatic way to repeat a loop 24 times is:

for ($i = 0; $i < 24; $i++) { ... }

as it includes the number 24. Starting from 0 but using the <= comparison is a bit odd.

The loops would be better written as

for ($h = 0; $h < 24; $h++) {
 for ($m = 0; $m < 60; $m++) {
 $time = sprintf('%02d:%02d', $h, $m);
 ...
 }
}
Source Link
200_success
  • 145.6k
  • 22
  • 190
  • 479

SQL

I would write the query as

SELECT t.`value` AS time_of_day, d.`sensor_id`
 FROM `times` t
 LEFT OUTER JOIN `data` d
 ON t.`mysql_hour_minute` = EXTRACT(HOUR_MINUTE FROM d.`datetime`)
 WHERE
 d.`datetime` BETWEEN '2014-11-05' AND '2014-11-05 23:59:59'
 AND d.`sensor_id` IN (1, 2, 3, 4, 5, 999)
 GROUP BY t.`value`, d.`sensor_id`
 HAVING COUNT(t.`value`) > 0
 ORDER BY d.`sensor_id` ASC, t.`value` ASC;

Note the following changes:

  • Changed RIGHT JOIN to LEFT OUTER JOIN, as left joins are more common and natural. Also, it matches the order in which the selected columns are presented.
  • Use the appropriate datetime function to avoid the LIKE operator for the join condition. Note that you'll have to add a column (which I've called mysql_hour_minute) to the times table, with integer values 0, 1, 2, ..., 59, 100, 101, ..., 159, 200, ..., 2359.
  • To accomplish WHERE d.datetime LIKE '...', the database would need to stringify every item. Stringifying is expensive in the first place; having to do it for every row is even worse. If you use a BETWEEN operator, then it can take advantage of an index. (And I hope you do have an index on the datetime column.)
  • If all you need to know is whether the sensors collected any data in each time slot, you could just put a HAVING condition on the query instead of requesting the count.

In addition, based on the fact that you accidentally left a $ in the example date, I suspect that you are composing the query by string interpolation, rather than by parameter substitution. You might have an SQL injection vulnerability.

PHP

A more idiomatic way to repeat a loop 24 times is:

for ($i = 0; $i < 24; $i++) { ... }

as it includes the number 24. Starting from 0 but using the <= comparison is a bit odd.

The loops would be better written as

for ($h = 0; $h < 24; $h++) {
 for ($m = 0; $m < 60; $m++) {
 $time = sprintf('%02d:%02d', $h, $m);
 ...
 }
}
default

AltStyle によって変換されたページ (->オリジナル) /