Ajax Voting System

Question

Everything works, but my strong suit definitely isn't JavaScript or jQuery. I'm sure there is some way that this can be accomplished that doesn't look so hacky.

HTML:

<div class="pull-left small">
 <a href="" class="vote" name="upvote" id="25">
 <i class="fa fa-thumbs-o-up" aria-hidden="true"></i>
 </a>
 <span id="upvote-25" class="text-success">76</span>
 <a href="" class="vote" name="downvote" id="25">
 <i class="fa fa-thumbs-o-down" aria-hidden="true"></i>
 </a>
 <span id="downvote-25" class="text-danger">0</span> 
</div>

jQuery:

$(document).ready(function () {
 $('.vote').click(function () {
 var vote_id = $(this).attr('name') + '-' + $(this).attr('id');
 var vote = +$('#' + vote_id).text();
 $('#' + vote_id).text(vote + 1)
 var data = {
 id: $(this).attr('id'),
 name: $(this).attr('name')
 };
 $.ajax({
 url: "php/ajax/setVote.php",
 dataType: "JSON",
 type: "POST",
 data: data
 });
 return false;
 });
});

PHP:

$iRowID = $_POST['id'];
$iVote = $_POST['name'];
$getVote = $dbh->prepare('
 SELECT upvote, downvote
 FROM blog
 WHERE id = :id;
');
$getVote->bindValue(':id', $iRowID);
if ($getVote->execute()) {
 while ($iRows = $getVote->fetch(PDO::FETCH_ASSOC)) {
 $upvote = $iRows['upvote'];
 $downvote = $iRows['downvote'];
 }
}
if ($iVote === "upvote") {
 $vote = $upvote + 1;
 $setVote = $dbh->prepare('
 UPDATE blog 
 SET upvote = :vote
 WHERE id = :id;
 ');
 $setVote->bindValue(':id', $iRowID);
 $setVote->bindValue(':vote', $vote);
 $setVote->execute();
}
if ($iVote === "downvote") {
 $vote = $downvote + 1;
 $setVote = $dbh->prepare('
 UPDATE blog 
 SET downvote = :vote
 WHERE id = :id;
 ');
 $setVote->bindValue(':id', $iRowID);
 $setVote->bindValue(':vote', $vote);
 $setVote->execute();
}

• Sᴀᴍ Onᴇᴌᴀ: Please don't update the code in the question. Refer to the section **What should I not do?**on What should I do when someone answers my question?
• indymx: Gotcha... Thanks for the heads up on that.

Answer 1

I'm only going to address the PHP and SQL code and the general voting mechanism.

I can hardly imagine that multiple blog items share the same id. Therefor, the while loop is unnecessary.

So, to get the amount of upvotes and downvotes for a particular blog item you could simply do:

$getVotes = $db->prepare( '
 SELECT upvote, downvote
 FROM blog
 WHERE id = :id
 LIMIT 1;
' );
// let's make sure preparing succeeded
if ( $getVotes ) {
 $getVote->bindValue( ':id', $iRowID );
 if ($getVote->execute() && $row = $getVote->fetch(PDO::FETCH_ASSOC)) {
 $upvote = $row['upvote'];
 $downvote = $row['downvote'];
 }
}

However, the whole voting routine is unnecessarily complicated and can be greatly simplified.

For one, it's unnecessary to (pre-)calculate the total votes in PHP, as this can easily be done in SQL. Therefor, it's not even necessary to fetch the votes first.

Secondly, it's not necessarily needed to have separate update statements for upvotes and downvotes, although I can imagine it could help grasping the logic better, perhaps.

Having said this, here's a way to greatly condense all your PHP code:

$id = $_POST['id'];
$voteType = $_POST['name'];
// make sure we received a valid vote type
if( $voteType === 'upvote' || $voteType === 'downvote' ) {
 $updateStatement = $dbh->prepare( '
 UPDATE blog
 SET upvote = upvote + :upvote, # add :upvote to current value of upvote
 downvote = downvote + :downvote # add :downvote to current value of downvote
 WHERE id = :id
 ' );
 // let's make sure preparing succeeded
 if( $updateStatement ) {
 $updateStatement->bindValue( ':id', $id );
 // increment by 1 if vote type is 'upvote' else leave unchanged (increment by 0)
 $updateStatement->bindValue( ':upvote', $voteType === 'upvote' ? 1 : 0 );
 // increment by 1 if vote type is 'downvote' else leave unchanged (increment by 0)
 $updateStatement->bindValue( ':downvote', $voteType === 'downvote' ? 1 : 0 );
 $updateStatement->execute();
 }
}

As you can see, we no longer fetch the votes first and increment them in PHP; we just let the database take care of that. We also just use one update statement to handle both vote type cases.

• indymx: I had never thought of doing it that way.
• indymx: Too bad that doesn't actually work. It was causing an Invalid parameter number error.
• Decent Dabbler: @indymx The errors are probably caused by the comments in the SQL statement (after the #). They aren't being interpreted as comments by your database engine. What flavor of database are you using? But the comments were only there for annotation purposes anyway, so you should just remove them from the actual code.
• indymx: That's possible I suppose. I believe the Db is MySQL. Could be Maria.. It's hosted on Godaddy. 5.6.36-cll-lve - MySQL Community Server (GPL)

Answer 2

Decent Dabbler, This is how I simplified the PHP. I'm not getting errors this way.

$id = $_POST['id'];
$iVote = $_POST['name'];
if ($iVote === 'upvote' || $iVote === 'downvote') {
 if ($iVote === "upvote") {
 $sql = 'UPDATE blog SET upvote = upvote + 1 WHERE id = :id';
 } elseif ($iVote === "downvote") {
 $sql = 'UPDATE blog SET downvote = downvote + 1 WHERE id = :id';
 }
 $setVote = $dbh->prepare($sql);
 if ($setVote) {
 $setVote->bindValue(':id', $id);
 $setVote->execute();
 }
}

• Decent Dabbler: Looks great @indymx. I wanted to suggest something similar as well, but opted for my other suggestion. But now, having seen it, I actually like your solution better than mine, because it's less prone to cause confusion.
• indymx: That was my thinking also. Plus if there is an error it might be easier to diagnose.

Answer 3

JavaScript

You can start by caching references to $(this) in a variable (e.g. elementRef), so as to avoid DOM-lookups each time you need a reference to the element.

$('.vote').click(function () {
 var elementRef = $(this);
 var vote_id = elementRef.attr('name') + '-' + elementRef.attr('id');
 var vote = +$('#' + vote_id).text();
 $('#' + vote_id).text(vote + 1)
 var data = {
 id: elementRef.attr('id'),
 name: elementRef.attr('name')
 };
}

See a demonstration here.

For more context, refer to the article below. I know it comes off in the beginning as tough on jQuery but it has some useful information.

Stop writing Slow Javascript.

PHP

Should anything be returned from the PHP to the client side? Perhaps it would at least be useful to have the front ent be aware that the request to the server-side was successful...

It appears that $upvote and $downvote May get overwritten between row iterations... If there is only supposed to be one row, perhaps using $getVote->fetchAll() would be a better solution than using a while loop- or if you just expect one row, remove the while. Also, if one row is expected, the database query could be updated to limit results to a single row - e.g. using LIMIT with MySQL, TOP with T-SQL/SQL Server, etc.

And are you intending to use hungarian notation for the variable naming convention? If not, what is the convention?

Update

I see you added an answer for the PHP simplification. I regret not thinking about that aspect in the past (I expect I would have concluded the two queries could be reduced to one if I had just stepped back and thought about it), and believe it can be simplified even more. That is because the field to update can be substituted directly from the variable (i.e. $iVote), and in_array() can be used to check that the value of $iVote matches one of the two column names instead of using two disjoint, separate conditions.

$id = $_POST['id'];
$iVote = $_POST['name'];
if (in_array($iVote, ['upvote', 'downvote'])) {
 $sql = 'UPDATE blog SET '. $iVote .' = '. $iVote . ' + 1 WHERE id = :id';
 $setVote = $dbh->prepare($sql);
 if ($setVote) {
 $setVote->bindValue(':id', $id);
 $setVote->execute();
 }
}

Or if you want to use a double-quoted string literal for the query then the variable could be parsed without needing to end and restart the string literal:

$sql = "UPDATE blog SET $iVote = $iVote + 1 WHERE id = :id";

• indymx: Interesting article. I've changed my code per your suggestion. Anything else you see that causes you to cringe?
• indymx: Sam, the naming convention is my own. sort of based on hungarian notation. The PHP should not return anything. It's simply recording the "vote" in the database. I didn't want to also return it and have to fumble around with updating the page from the PHP. simple enough to just increment the vote client side. Seemed lest costly to me that way. As there is only on iteration per click on the PHP, I don't think the $upvote and $downvote would get overwritten.
• Sᴀᴍ Onᴇᴌᴀ: Okay - good to know. I updated the comment about $upvote and $downvote - perhaps the while statement isn't needed if there is only one row expected from the query...
• indymx: Yeah, I suppose I could simplify that.
• Sᴀᴍ Onᴇᴌᴀ: I added more to the PHP portion.