5
\$\begingroup\$

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();
}
\$\endgroup\$
4
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I had never thought of doing it that way. \$\endgroup\$ – indymx Nov 30 '17 at 10:46
  • \$\begingroup\$ Too bad that doesn't actually work. It was causing an Invalid parameter number error. \$\endgroup\$ – indymx Nov 30 '17 at 11:58
  • \$\begingroup\$ @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. \$\endgroup\$ – Decent Dabbler Nov 30 '17 at 16:31
  • \$\begingroup\$ 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) \$\endgroup\$ – indymx Nov 30 '17 at 16:35
2
\$\begingroup\$

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();
    }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Decent Dabbler Nov 30 '17 at 16:37
  • 1
    \$\begingroup\$ That was my thinking also. Plus if there is an error it might be easier to diagnose. \$\endgroup\$ – indymx Nov 30 '17 at 16:38
2
\$\begingroup\$

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";
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Interesting article. I've changed my code per your suggestion. Anything else you see that causes you to cringe? \$\endgroup\$ – indymx Nov 29 '17 at 2:04
  • \$\begingroup\$ 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. \$\endgroup\$ – indymx Nov 29 '17 at 11:35
  • \$\begingroup\$ 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... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 29 '17 at 18:03
  • \$\begingroup\$ Yeah, I suppose I could simplify that. \$\endgroup\$ – indymx Nov 29 '17 at 18:19
  • \$\begingroup\$ I added more to the PHP portion. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Dec 7 '17 at 21:41

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.