Vote-switching optimization

Question

I have 2 <label>s being used for displaying the votes, which is up votes and down votes. There is no problem for the adding or removing votes, but the switching of votes is the issue (not really an issue).

The structure of the votes in the HTML is:

<label id="up_vote"></label>
<label id="down_vote"></label>

The values are retrieved from the database and I user innerHTML to insert the data between the <label> tags. Once the user clicks the <label>, either the up_vote or the down_vote, the update request is sent through ajax and updates the vote counts, and once it is successfully executed, the value inside the <label> will be incremented by one, as well as the removing of vote, it will be decremented. And a voted class will be added once the user votes.

My JavaScript looks like this:

$("#up_vote, #down_vote").click(function(e) {
  var voteCount = document.getElementById(e.target.id).innerHTML;
  var action;
  var value;

  if ($("#up_vote").hasClass("voted") || $("#down_vote").hasClass("voted")) {
    if ($(this).hasClass("voted")) {
      action = "remove_vote";
      setValue();
    } else {
      action = "switch_vote";
      setValue();
    }
  } else {
    action = "add_vote";
    setValue();
  }

  function setValue() {
    if (e.target.id === "up_vote") {
      value = 1;
    } else if (e.target.id === "down_vote") {
      value = -1;
    }
  }

  $.ajax({
    method: "GET",
    url: "votes.php",
    data: , //it includes the user session id, the post id, and the action and values for doing the specific functions (i just didn't put it here, just giving the logic on what does my code looks like
    cache: false,
    success: function(msg) {
      if (msg === "vote_added") {
        document.getElementById.innerHTML = parseInt(voteCount) + 1;
        $("#" + e.target.id).addClass("voted");
      } else if (msg === "vote_removed") {
        // the same function of the vote_added message, but instead, it decrements the value and removes the "voted" class
      } else if (msg === "vote_switched") {
        // these is the part that i want to achieve, but coding it seems a bit long (just my thought)
      } else if (msg === "vote_exist") {
        // these is a, some sort of notifying the user that the he/she already votes the specific post
      }
    }
  })
})

As on what you see on the JavaScript, it sends an action and value (which will specify if it is up vote or down vote, which will be used to do specific functions inside the PHP. It will then echo out the message that will be used once the query is executed successfully.

(On the last condition, vote_exist, is used for security check. While I'm testing something out, removing the class="voted" (which notifies that the user already voted) inside Inspector window, clicking these back will increment the vote count again, which removes the restrictions, so I added a validation function inside the PHP before the user votes for doubled security check. Hope that these can help for others.)

My concern is about the 3rd condition, which is the switching of votes. For example, the user already up votes, and then he/she wants to down vote. There is no problem for the queries, just for the JavaScript part. I have an idea, though: by using the same functions inside the vote_added and vote_removed, where, when clicked, will target id will be incremented at the same time the other one will be decremented, also adds a voted class to the target id and removes the other one.

How can I do this in a shorter way?

Answer 1

We cant review the code if you don't provide it, telling us that it is a bit long will not help. You should provide the full code especially since it is likely that refactoring will be useful between the different cases.

Regardless, here are some observations from what you did provide:

• In essence, whatever the user does, you will always call setValue(), once you realize this you can simplify the first if statement

  if ($("#up_vote").hasClass("voted") || $("#down_vote").hasClass("voted")) {
    if ($(this).hasClass("voted")) {
      action = "remove_vote";
    } else {
      action = "switch_vote";
    }
  } else {
    action = "add_vote";
  }
  setValue();
  

• And, since you only call setValue() once, you can even take it out of it's function and just put it after the first if statement.

• You might even consider replacing the if statement inside setValue with a ternary statement: value = e.target.id === "up_vote" ? 1 : -1;

• When posting data, you should use "POST", not "GET"

• I feel votes.php should give back the vote counts. Just adding 1 or removing 1 from the last vote count is cheating, and I think users can tell.

• Finally, and this is a matter of taste, I would not send +1 or -1 back, I would send the value of e.target.id back, and let the back-end do the conversion.