0
\$\begingroup\$

I have the following table:

id   speed   date
1    0       01/01/2015     
2    0       01/01/2015 
3    0       01/01/2015 
4    0       01/01/2015 
5    0       01/01/2015 
6    0       01/01/2015 
7    25      01/01/2015 
8    78      01/01/2015 
9    13      01/01/2015 
10   45      01/01/2015 
11   0       01/01/2015 
12   80      01/01/2015 
13   86      01/01/2015 
14   10      01/01/2015 
15   0       01/01/2015 
16   0       01/01/2015 
17   0       01/01/2015 
18   0       01/01/2015

This is just a small part of the table, it has many dates. Each new date starts and ends with an unknown amount of zeros. And I have to skip these zeros and get only what's between them. So I came up with the following solution:

  1. Get the ID of the first row whose speed is positive (in our table it's id number 4).

  2. Get the ID of the last row whose speed is positive (in our table it's id number 12).

  3. Get lines between these two IDs (4 and 12).

I get the id of the first row whose speed is positive:

$q1=$conn->prepare("SELECT * FROM tableX WHERE speed > 0 order by date asc LIMIT 1");
$q1->execute();
$r1=$q1->fetch();
$first_id = $r1['id'];

And then get the id of the last row whose speed is positive:

$q2=$conn->prepare("SELECT * FROM tableX WHERE speed > 0 order by date desc LIMIT 1");
$q2->execute();
$r2=$q2->fetch();
$last_id = $r2['id'];

And then use those ids to get what I want:

$q3=$conn->prepare("SELECT * FROM tableX WHERE id between '$first_id' and '$last_id'");
$q3->execute();
while($r3=$q3->fetch(){}

Expected result:

id   speed   date
7    25      01/01/2015 
8    78      01/01/2015 
9    13      01/01/2015 
10   45      01/01/2015 
11   0       01/01/2015 
12   80      01/01/2015 
13   86      01/01/2015 
14   10      01/01/2015

My code is actually working but I think this solution of mine is lame, so I am looking for an improvement, kind of all these three queries in one!

\$\endgroup\$
  • 1
    \$\begingroup\$ Your code has a random element with the data given, so that when you order it by date asc it is kind of random which row with speed > 0 you'll get. In the data given, all rows have the same date, and it depends on the database implementation which row id you'll get first or last. As such this code is broken, or dependent on your choice of database engine and their choice of returning rows when the order column is identical \$\endgroup\$ – holroy Nov 16 '15 at 17:11
  • \$\begingroup\$ I'd recommend rephrasing your question to be "a better way" or "improvements to this way" instead of "another way". This is Code Review, not Give Me Code For Free. \$\endgroup\$ – Nic Hartley Nov 16 '15 at 22:45
  • \$\begingroup\$ You said "most of the time". So what happens (or what should happen) when the series is not bounded by zeroes? \$\endgroup\$ – 200_success Nov 17 '15 at 7:06
  • \$\begingroup\$ Your $r3 line also doesn't make sense. \$\endgroup\$ – 200_success Nov 17 '15 at 7:07
  • \$\begingroup\$ Putting the question on hold due to quality concerns noted above by holroy and me. \$\endgroup\$ – 200_success Nov 17 '15 at 7:10
1
\$\begingroup\$

First of all a style comment: Please use longer descriptive names. Stuff like $q3, $q2, is just ugly, and confusing.

You could simplify your query either by using subqueries in the WHERE clause, or by creating a temporary table in the FROM clause. You could use the BETWEEN operator, but some think BETWEEN is evil as it is somewhat unclear from the wording whether the range is inclusive or exclusive. In addition if used on date fields, you could have various interesting rounding errors. So it better to stick with ordinary operators.

I've commented upon the fact that you'll have a potential issue related to order, and I haven't compensated for that in the following code, only assuming that rows are indeed ordered by id. This could/should possibly be enforced. Here is the SQL code to get your rows in one go:

SELECT tableX.* FROM tableX,
  ( SELECT min(id) as startId, max(id) as endId FROM tableX
     WHERE speed > 0 AND date = '2015-01-01' ) tmp
 WHERE id >= tmp.startId AND id <= tmp.endId

Note that in the temporary table select I chosen a given date, instead of ordering by date (which most likely would throw of the id part quite easily. Another option could be to specify a double order like in ORDER BY date, id.

Untested version translated into php-code:

$speed_query = $db_connection->prepare(
   "SELECT tableX.* FROM tableX, 
     ( SELECT min(id) as startId, max(id) as endId FROM tableX 
        WHERE speed > 0 AND date = '$date' ) tmp 
   WHERE id >= tmp.startId AND id <= tmp.endId ");
$speed_query->execute();
while($row = $speed_query->fetch()) {
   ... do something useful with $row ...
}

The advantage of this method is that you now get all of your data from one single query, and one roundtrip from your code base to the database engine. Using subqueries like this shouldn't be to expensive, and at least when comparing to making several roundtrips it is a way better solution.

PS! You could consider using heredocs or other variations to get the multiline string in your query. But do try to break it over multiple lines to enhance the readability of the SQL query. See PHP string doc for various option on how to use strings in PHP.

\$\endgroup\$
0
\$\begingroup\$

So, your first two functions are correct but unnecessary for thing you want to do.

You successfully get ID of first and last row where 'speed' is larger than 0. And that's okay, but not needed.

But problem is here:
$q3=$conn->prepare("SELECT * FROM tableX WHERE id between '$first_id' and '$last_id'");

This function will get all rows between first and last id but it doesn't care what is value of 'speed'.

Now, let's examine this fixed code:

$q2=$conn->prepare("SELECT * FROM tableX WHERE speed > 0");

This one will get all rows where 'speed' is larger than 0.
It will look through every row and check column 'speed' value.

If you want ordering, you just change it to
$q2=$conn->prepare("SELECT * FROM tableX WHERE speed > 0 ORDER BY //-SOME ROW-// ASC"); (or DESC)
And you can safely delete those two functions that get first and last row id.

\$\endgroup\$
  • \$\begingroup\$ Welcome to Code Review! You have presented an alternative solution, but haven't reviewed the code. Please explain your reasoning (how your solution works and how it improves upon the original) so that the author can learn from your thought process. \$\endgroup\$ – SuperBiasedMan Nov 16 '15 at 17:33
  • 1
    \$\begingroup\$ @SuperBiasedMan Thank you for welcome. I should have firstly read rules and I will edit my answer. \$\endgroup\$ – Nikola Miljković Nov 16 '15 at 17:36
  • 1
    \$\begingroup\$ The OP actually wants row 11 to be included in result (which has speed = 0 so you can't just simply use speed > 0. This kind of invalidates your answer entirely... \$\endgroup\$ – holroy Nov 18 '15 at 23:53
  • \$\begingroup\$ I'm not sure if OP wants row 11 to be included, actually, I think that was bug in his code... \$\endgroup\$ – Nikola Miljković Nov 19 '15 at 8:59
  • 1
    \$\begingroup\$ @SomeHelpingDude No it's not a bug i actually want it incuded as you can see in the result. \$\endgroup\$ – 0x58 Nov 19 '15 at 9:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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