0

I have the following code that works and does what I want it to but I feel I'm using more code than is necessary. All I want to do is get the value in a database cell and check if it is '1' and if so run another query.

$isComplete = $database -> prepare("SELECT completed FROM projects WHERE id = $project_id");
$isComplete -> execute();
$result = $isComplete -> fetchAll();
$result = count($result);
if($result == 1) { $database -> exec("UPDATE projects SET num_complete = num_complete - 1 WHERE id = $parent_id"); }
Craig Harshbarger
  • 1,863
  • 3
  • 18
  • 29
  • join or subquery in the update would be preferable, no need for the select –  Aug 23 '13 at 03:29
  • Interpolating a value into a statement defeats one of the main benefits of prepared statements, namely prepared statement parameters. – outis Aug 23 '13 at 03:48
  • @outis, yes I know. Thank you for pointing that out. I was using just a execute statement and I copied it into a prepared statement so I could assign the result to a variable forgetting to bind in the value. – Craig Harshbarger Aug 23 '13 at 04:23
  • To make the [sample code](http://sscce.org/) complete, the question should include table schema and sample data (as SQL, of course). Using `fetchAll` and `count()` in PHP is inefficient; using MySQL's `COUNT()` will be more performant. – outis Aug 23 '13 at 11:19

3 Answers3

0

First, your code is indeed non optimal from the amount of code point of view.
It is also contradicts with your own description

And - worse of all - it is prone to SQL injection.

Also, your variable naming is inconsistent and confusing.
Here is the proper code to check if selected value = 1.

$stmt = $database->prepare("SELECT completed FROM projects WHERE id = ?");
$stmt->execute(array($project_id));
$isComplete = $stmt->fetchColumn();
if ($isComplete) ...

However, I doubt you need such a code at all. To get the number of completed subtasks is a matter of one simple query. Are you really sure you need this num_complete field at all?

Community
  • 1
  • 1
Your Common Sense
  • 156,878
  • 40
  • 214
  • 345
  • Yes it was pointed out above by outis that the variable wasn't being bound. I think you're right about not needing the `num_complete` field. I figured out that this query does work though `UPDATE projects SET num_complete = num_complete - 1 WHERE id = :parent_id and (select * from (select completed from projects where id = :project_id) x)= 1` is this proper/safe/effective code? It uses a nested subquery to get around sql not letting you select from a database that is being updated. – Craig Harshbarger Aug 23 '13 at 14:45
0

Instead of a subquery in the WHERE clause, you can put one in a JOIN clause to get around MySQL's limitation against targeting a table both in an UPDATE and a subquery. Also, instead of selecting all rows with a given project ID and counting them in PHP, you can perform the calculation in the SQL query. Something like:

UPDATE projects p0
    JOIN (SELECT id, count(*) AS nSiblings
            FROM projects
            WHERE id=:project GROUP BY id)
         AS p1
      ON p0.id=p1.id
  SET p0.num_complete=p0.num_complete+1
  WHERE p1.nSiblings=1

Note that since it's an inner join, specifying the ID in the subquery is sufficient. You could also probably drop the GROUP BY id, but if you ever adapt the statement for another use, it could introduce a bug.

There may be other issues with the table design that affect this query (and other aspects), but since no schema was provided, there's no way to provide feedback.

outis
  • 75,655
  • 22
  • 151
  • 221
  • Would this query also be a good option? Or is this bad coding... `UPDATE projects SET num_complete = num_complete - 1 WHERE id = :parent_id and (select * from (select completed from projects where id = :project_id) x)= 1` – Craig Harshbarger Aug 23 '13 at 23:05
-1

You can use a subquery to check the condition in the update statement. Something like this:

UPDATE projects SET num_complete = num_complete - 1 WHERE id = $parent_id
and (select completed from projects where id = $project_id) = 1

For example:

$st = $database->prepare("UPDATE projects SET num_complete = num_complete - 1 WHERE id = :parent_id
    and (select completed from projects where id = :project_id) = 1");
$st->bindParam(':parent_id', $parent_id, PDO::PARAM_INT);
$st->bindParam(':project_id', $project_id, PDO::PARAM_INT);
$st->execute();
Arsen
  • 965
  • 8
  • 7
  • Yes, but only for the proper id and when the project with project_id passed in is completed. – Arsen Aug 23 '13 at 04:26
  • Ahh I should have known that... Yet it never crossed my mind. Thank you! I commented out the lines of code in my original post and added this `$database -> exec("UPDATE projects SET num_complete = num_complete - 1 WHERE id = $parent_id and (select completed from projects where id = $project_id) = 1");` but its not working. I understand the logic but its not affecting any rows in the database. – Craig Harshbarger Aug 23 '13 at 04:32
  • I think you aren't passing the parameters needed for the $parent_id and $project_id to your prepare statement. http://php.net/manual/en/pdostatement.bindparam.php – Arsen Aug 23 '13 at 04:37
  • Edited the answer. What is the value of the "completed" field? Is it 1. In your original case, you weren't checking the value of the "completed" column you were simply seeing if there was exactly one row in the projects table with that project id irrespective of the value of the completed field. – Arsen Aug 23 '13 at 04:41
  • Yes it is 1 if it is completed and 0 if it is unfinished. if it is 1 then i want to minus 1 from the num_complete field of parent_id's row – Craig Harshbarger Aug 23 '13 at 04:56
  • I ran the query on my database adding in the id's of the parent and child. I received this error `You can't specify target table 'projects' for update in FROM clause` – Craig Harshbarger Aug 23 '13 at 05:01
  • "Currently, you cannot delete from a table and select from the same table in a subquery" - [Source](http://stackoverflow.com/a/4486491/2687861) so that query you gave me won't work? Hmmmm now idk what to think lol – Craig Harshbarger Aug 23 '13 at 05:12