pritam79 Posted July 14, 2009 Share Posted July 14, 2009 I am trying the poll script using MySql with PHP, this is what I have done so far.This is the poll.htm file <html><head></head><body><div id="poll"><h3>Do you like PHP and AJAX so far?</h3><form action="poll_vote.php" method="post">Yes:<input type="radio" name="vote" value="1"><br>No:<input type="radio" name="vote" value="0"><input type="submit"></form></div></body></html> And this is the poll_vote.php- <?php$vote = $_POST['vote'];$con=mysql_connect("localhost","root","");if (!$con) { die('Could not connect: ' . mysql_error()); } mysql_select_db("poll");$result=mysql_query("SELECT * FROM poll_result");while($row = mysql_fetch_array($result)) { if($vote == 1) { $row['Yes'] += 1; // increment values in the database } if($vote == 0) { $row['No'] += 1; } }mysql_query("UPDATE poll_result SET YES = '$row['Yes']' AND NO = '$row['No']'");while($row = mysql_fetch_array($result)) { echo "<p>Total votes= ".($row['Yes'] + $row['No'])."</p>"; echo "<p>Yes= ".$row['Yes']."</p>"; echo "<p>No= ".$row['No']."</p>"; }?> On selecting either Yes or No option from the html form, I am getting this error- Parse error: parse error, expecting `T_STRING' or `T_VARIABLE' or `T_NUM_STRING' in C:\wamp\www\poll\poll_vote.php on line 27. The database is not getting updated anyway, but is there anything wrong with the script? Also please suggest the simplest form of this same script using PHP MySQL. I would also like to know where I am going wrong with my current scripts. Thanks…… Link to comment Share on other sites More sharing options...
Ingolme Posted July 14, 2009 Share Posted July 14, 2009 I'd put the variables between braces just to be sure:mysql_query("UPDATE poll_result SET YES = '{$row['Yes']}' AND NO = '{$row['No']}'");And I'm not sure exactly what you're trying to do with that query. You can only use AND after the WHERE clause.I think you want to do this: UPDATE poll_result SET YES = '{$row['Yes']}', NO = '{$row['No']}' Link to comment Share on other sites More sharing options...
pritam79 Posted July 15, 2009 Author Share Posted July 15, 2009 I'd put the variables between braces just to be sure:mysql_query("UPDATE poll_result SET YES = '{$row['Yes']}' AND NO = '{$row['No']}'");And I'm not sure exactly what you're trying to do with that query. You can only use AND after the WHERE clause.I think you want to do this:UPDATE poll_result SET YES = '{$row['Yes']}', NO = '{$row['No']}' After using mysql_query("UPDATE poll_result SET YES = '{$row['Yes']}', NO = '{$row['No']}'");i get no errors but the database doesn't get updated. Plz tell me whats wrong with the code? What i want is the 'yes' and 'no' votes to get incremented in the database as the user selects the option. Link to comment Share on other sites More sharing options...
Synook Posted July 15, 2009 Share Posted July 15, 2009 Count your brackets. Also, do you really want to increment the vote for all your records?However, why not just UPDATE poll_results SET Yes = Yes + 1, No = No + 1 WHERE id = $whatever By the way, field names are case-sensitive in MySQL. Link to comment Share on other sites More sharing options...
dsonesuk Posted July 15, 2009 Share Posted July 15, 2009 would it be better to do it this way?while($row = mysql_fetch_array($result)) { $new_no = $row['No']; $new_yes= $row['Yes']; if($vote == 1) { $new_yes+=1; } if($vote == 0) { $new_no+=1; } }then:mysql_query("UPDATE poll_result SET Yes = '$new_yes', No= '$new_no'"); Link to comment Share on other sites More sharing options...
justsomeguy Posted July 15, 2009 Share Posted July 15, 2009 It's not going to matter much for small amounts of users, but for a lot of users, it will be better for concurrency to do like Synook showed, to just add one to the column in the database. Getting the value like you showed and incrementing it, then writing that value back, may introduce a race condition if a lot of people are using it at once where some votes might not get counted. But like I said, if not a lot of people are using it then it probably won't make a difference. Link to comment Share on other sites More sharing options...
pritam79 Posted July 16, 2009 Author Share Posted July 16, 2009 Thanks, it worked Link to comment Share on other sites More sharing options...
pritam79 Posted July 21, 2009 Author Share Posted July 21, 2009 It's not going to matter much for small amounts of users, but for a lot of users, it will be better for concurrency to do like Synook showed, to just add one to the column in the database. Getting the value like you showed and incrementing it, then writing that value back, may introduce a race condition if a lot of people are using it at once where some votes might not get counted. But like I said, if not a lot of people are using it then it probably won't make a difference.What kind of race condition could that bring about? Any solutions for its prevention? Link to comment Share on other sites More sharing options...
justsomeguy Posted July 21, 2009 Share Posted July 21, 2009 It's all about timing. A CPU can only do work for one process at a time, so the web server might have several processes running that are servicing the various requests. The CPU constantly switches between the processes to make sure that all processes stay responsive and none of them freeze up because the CPU is too busy. So it will do some work for one process for a while, then put that on hold and switch to another process and do some work there, then switch again, etc. The part of the operating system responsible for that is called the process scheduler, the point of it is to make sure that each process gets CPU time.So, imagine if two people submitted votes at nearly the same time. So the web server might have 2 processes running, one for each user, where it's processing their votes. So let's assume that this is the code that each process is running: $result=mysql_query("SELECT * FROM poll_result");if($row = mysql_fetch_array($result)) { if($vote == 1) { $row['Yes'] += 1; // increment values in the database } if($vote == 0) { $row['No'] += 1; }}mysql_query("UPDATE poll_result SET YES = '{$row['Yes']}', NO = '{$row['No']}'"); So each process gets the vote totals from the database, adds one to whatever choice they chose, and updates the database with the new total. So these two users submit at nearly the same time, so the process for user 1 starts running and it gets the vote totals from the database. Maybe the database is taking a while, so as soon as the database result comes back the scheduler switches to the process for user 2, which also goes to get the totals from the database. At this point, the data for each process is the same. Let's say the totals are 12 yes votes and 8 no votes, well since both processes got the totals from the database, each process thinks 12 yes votes and 8 no votes have been cast. So, user 1 voted yes, so it updates that item in the row to 13, and then updates the database. So now the database has 13 votes for yes, and 8 votes for no. So it switches back to process 2, where the results it got from the database are still 12/8. So since user 2 voted no, it adds 1 to that column and writes back to the database. So now both processes have finished, both users have voted, and the database shows 12 yes votes and 9 no votes, because that's what process 2 wrote back to the database. So it essentially lost the vote for user 1.The solution for that is to update the database directly. Instead of getting the totals, adding 1, and writing back, just use a single update statement to add 1 to the column, the way Synook showed. If the code is this: if($vote == 1) { mysql_query('UPDATE poll_result SET Yes = Yes + 1'); } if($vote == 0) { mysql_query('UPDATE poll_result SET No = No + 1'); } No matter which order the processes run in, and no matter when they stop and start, it's always going to count the votes. Each process just adds 1 to the column regardless of what the current value is, so it's never going to drop a vote. Link to comment Share on other sites More sharing options...
pritam79 Posted July 23, 2009 Author Share Posted July 23, 2009 The solution for that is to update the database directly. Instead of getting the totals, adding 1, and writing back, just use a single update statement to add 1 to the column, the way Synook showed. If the code is this: if($vote == 1) { mysql_query('UPDATE poll_result SET Yes = Yes + 1'); } if($vote == 0) { mysql_query('UPDATE poll_result SET No = No + 1'); } Hi there, my poll script worked when I did it the way 'dsonesuk' showed but the race condition is also something that should strictly be considered. This is the modified poll_vote.php script—<?php$vote = $_POST['vote']; // $vote is 1 if YES is selected // and $vote is 0 if NO is selected $con=mysql_connect("localhost","root","");if (!$con) { die('Could not connect: ' . mysql_error()); } mysql_select_db("poll");$result=mysql_query("SELECT * FROM poll_result");while($row = mysql_fetch_array($result)) { $no = $row['No']; $yes= $row['Yes']; if($vote == 1) { mysql_query("UPDATE poll_result SET Yes = Yes + 1"); } if($vote == 0) { mysql_query("UPDATE poll_result SET No = No + 1"); }?><html><body bgcolor="silver"><center>TOTAL VOTES-<?php echo $yes + $no; ?><br><br><br><table><tr><td>Yes votes:<?php echo $yes;?></td><td><img src="poll.gif" width='<?php echo(100*round($yes/($no+$yes),2)); ?>' height='20'><?php echo(100*round($yes/($no+$yes),2)); ?>%</td></tr><br><tr><td>No votes:<?php echo $no; ?></td><td><img src="poll.gif"width='<?php echo(100*round($no/($no+$yes),2)); ?>' height='20'><?php echo(100*round($no/($no+$yes),2)); ?>%</td></tr></table></center></body></html> But now I am getting this error Parse error: parse error in C:\wamp\www\poll\poll_vote.php on line 52 This is what happened after implementing your code. Please help… Link to comment Share on other sites More sharing options...
justsomeguy Posted July 23, 2009 Share Posted July 23, 2009 The while loop block never ends, there's not a closing bracket for it.One thing I'll say is to remove the while loop, if there is more than one row in the poll_results table, you're going to increment the vote count once for each row because the code to increment is inside a loop. A while loop is supposed to be used to loop over all of the rows in a result set, I see a lot of people use a while loop regardless of what's coming back. If you're only looking for one row, there's no reason to loop over everything. It's better to use an if statement and that way you can also check if no rows were returned and print an error message if so.You're also getting the results before adding the new vote, so the vote totals you're going to show were the totals before the current vote was cast. Link to comment Share on other sites More sharing options...
Recommended Posts
Archived
This topic is now archived and is closed to further replies.