Tips on how to refactor this unwieldy upvote/downvote code

Question

Basically this code is for an upvote/downvote system and I'm basically

• Incrementing the count by 1 when voting up
• Decrementing the count by 1 when voting down
• If the number of downvotes> upvotes, we'll assume it's a negative score, so the count stays 0
• Reverting the count back to what it originally was when clicking upvote twice or downvote twice
• Never go below 0 (by showing negative numbers);

Basically it's the same scoring scheme reddit uses, and I tried to get some ideas from the source which was minified and kind of hard to grok:

 a.fn.vote = function(b, c, e, j) {
 if (reddit.logged && a(this).hasClass("arrow")) {
 var k = a(this).hasClass("up") ? 1 : a(this).hasClass("down") ? -1 : 0, v = a(this).all_things_by_id(), p = v.children().not(".child").find(".arrow"), q = k == 1 ? "up" : "upmod";
 p.filter("." + q).removeClass(q).addClass(k == 1 ? "upmod" : "up");
 q = k == -1 ? "down" : "downmod";
 p.filter("." + q).removeClass(q).addClass(k == -1 ? "downmod" : "down");
 reddit.logged && (v.each(function() {
 var b = a(this).find(".entry:first, .midcol:first");
 k > 0 ? b.addClass("likes").removeClass("dislikes unvoted") : k < 0 ? b.addClass("dislikes").removeClass("likes unvoted") : b.addClass("unvoted").removeClass("likes dislikes")
 }), a.defined(j) || (j = v.filter(":first").thing_id(), b += e ? "" : "-" + j, a.request("vote", {id: j,dir: k,vh: b})));
 c && c(v, k)
 }
};

I'm trying to look for a pattern, but there are a bunch of edge cases that I've been adding in, and it's still a little off.

My code (and fiddle):

$(function() {
 var down = $('.vote-down');
 var up = $('.vote-up');
 var direction = up.add(down);
 var largeCount = $('#js-large-count');
 var totalUp = $('#js-total-up');
 var totalDown = $('#js-total-down');
 var totalUpCount = parseInt(totalUp.text(), 10); 
 var totalDownCount = parseInt(totalDown.text(), 10); 
 var castVote = function(submissionId, voteType) {
 /*
 var postURL = '/vote';
 $.post(postURL, {
 submissionId : submissionId,
 voteType : voteType
 } , function (data){
 if (data.response === 'success') {
 totalDown.text(data.downvotes);
 totalUp.text(data.upvotes);
 } 
 }, 'json');
 */
 alert('voted!');
 }; 
 $(direction).on('click', direction, function () { 
 // The submission ID
 var $that = $(this),
 submissionId = $that.attr('id'),
 voteType = $that.attr('dir'), // what direction was voted? [up or down]
 isDown = $that.hasClass('down'),
 isUp = $that.hasClass('up'),
 curVotes = parseInt($that.parent().find('div.count').text(), 10); // current vote
 castVote(submissionId, voteType);
 // Voted up on submission
 if (voteType === 'up') {
 var alreadyVotedUp = $that.hasClass('likes'),
 upCount = $that.next('div.count'),
 dislikes = $that.nextAll('a').first(); // next anchor attr
 if (alreadyVotedUp) {
 // Clicked the up arrow and previously voted up
 $that.toggleClass('likes up');
 if (totalUpCount > totalDownCount) {
 upCount.text(curVotes - 1);
 largeCount.text(curVotes - 1);
 } else {
 upCount.text(0);
 largeCount.text(0);
 } 
 upCount.css('color', '#555').hide().fadeIn();
 largeCount.hide().fadeIn();
 } else if (dislikes.hasClass('dislikes')) {
 // Voted down now are voting up
 if (totalDownCount > totalUpCount) {
 upCount.text(0);
 largeCount.text(0);
 } else if (totalUpCount > totalDownCount) {
 console.log(totalDownCount);
 console.log(totalUpCount);
 if (totalDownCount === 0) { 
 upCount.text(curVotes + 1);
 largeCount.text(curVotes + 1);
 } else {
 upCount.text(curVotes + 2);
 largeCount.text(curVotes + 2);
 } 
 } else {
 upCount.text(curVotes + 1);
 largeCount.text(curVotes + 1);
 } 
 dislikes.toggleClass('down dislikes');
 upCount.css('color', '#296394').hide().fadeIn(200);
 largeCount.hide().fadeIn();
 } else {
 if (totalDownCount > totalUpCount) {
 upCount.text(0);
 largeCount.text(0);
 } else {
 // They clicked the up arrow and haven't voted up yet
 upCount.text(curVotes + 1);
 largeCount.text(curVotes + 1).hide().fadeIn(200);
 upCount.css('color', '#296394').hide().fadeIn(200);
 } 
 } 
 // Change arrow to dark blue
 if (isUp) {
 $that.toggleClass('up likes');
 } 
 }
 // Voted down on submission
 if (voteType === 'down') {
 var alreadyVotedDown = $that.hasClass('dislikes'),
 downCount = $that.prev('div.count');
 // Get previous anchor attribute
 var likes = $that.prevAll('a').first();
 if (alreadyVotedDown) {
 if (curVotes === 0) {
 if (totalDownCount > totalUp) {
 downCount.text(curVotes);
 largeCount.text(curVotes);
 } else {
 if (totalUpCount < totalDownCount || totalUpCount == totalDownCount) {
 downCount.text(0);
 largeCount.text(0);
 } else {
 downCount.text((totalUpCount - totalUpCount) + 1);
 largeCount.text((totalUpCount - totalUpCount) + 1);
 }
 }
 } else {
 downCount.text(curVotes + 1);
 largeCount.text(curVotes + 1);
 }
 $that.toggleClass('down dislikes');
 downCount.css('color', '#555').hide().fadeIn(200);
 largeCount.hide().fadeIn();
 } else if (likes.hasClass('likes')) {
 // They voted up from 0, and now are voting down
 if (curVotes <= 1) {
 downCount.text(0);
 largeCount.text(0);
 } else {
 // They voted up, now they are voting down (from a number > 0)
 downCount.text(curVotes - 2);
 largeCount.text(curVotes - 2);
 }
 likes.toggleClass('up likes');
 downCount.css('color', '#ba2a2a').hide().fadeIn(200);
 largeCount.hide().fadeIn(200);
 } else {
 if (curVotes > 0) {
 downCount.text(curVotes - 1);
 largeCount.text(curVotes - 1);
 } else {
 downCount.text(curVotes);
 largeCount.text(curVotes);
 }
 downCount.css('color', '#ba2a2a').hide().fadeIn(200);
 largeCount.hide().fadeIn(200);
 }
 // Change the arrow to red
 if (isDown) {
 $that.toggleClass('down dislikes');
 }
 }
 return false;
 });
});​

Pretty convoluted, right? Is there a way to do something similar but in about 1/3 of the code I've written? After attempting to re-write it, I find myself doing the same thing so I just gave up halfway through and decided to ask for some help (fiddle of most recent).

Answer 1

Don't use an A element for the up/down buttons, use a button or styled span element.

The logic seems way more convoluted than necessary, consider something much simpler. The function below gets the original value, calculates the new value, then works out what to do with it. If it's within the range +/-1, it uses that value, otherwise it resets to the original value.

Use classes instead of IDs so that the function can be generic. I don't use jQuery, you can convert the logic if you want. This is dependent on the client having getElementsByClassName, it's pretty easy to add if it's missing. There are a few support functions at the start, they're simple and you should have your own small library of such functions already.

// Support functions
function hasClass(el, cName) {
 var re = new RegExp('(^|\\s)' + cName + '(\\s|$)');
 return re.test(el.className);
}
function getText(el) {
 if (typeof el.textContent == 'string') {
 return el.textContent;
 } else if (typeof el.innerText == 'string') {
 return el.innerText;
 }
}
function setText(el, text) {
 if (typeof el.textContent == 'string') {
 el.textContent = text;
 } else if (typeof el.innerText == 'string') {
 el.innerText = text;
 }
} 
// Add listeners onload 
window.onload = function() {
 var voteEls = document.getElementsByClassName('thingToBeVotedOn');
 for (var i=0, iLen=voteEls.length; i<iLen; i++) {
 voteEls[i].onclick = handleClick;
 }
 function handleClick(evt) {
 evt = evt || window.event;
 var node, scoreNode, originalScore, currentScore, newScore, diff;
 var el = evt.target || evt.srcElement;
 var increment = hasClass(el, 'up')? 1 : 
 hasClass(el, 'down')? -1 :
 0; // default
 // Thing to be voted on is the button's parentNode
 // Only do something if there is an increment
 if (increment) {
 node = el.parentNode;
 scoreNode = node.getElementsByClassName('count')[0];
 originalScore = Number(node.getAttribute('data-originalValue'));
 currentScore = Number(getText(scoreNode)) || 0;
 newScore = currentScore + increment;
 diff = Math.abs(originalScore - newScore);
 // Can't go more than + or - 1
 if (diff > 1) {
 newScore = originalScore;
 }
 // Set new displayed value
 setText(scoreNode, newScore);
 } 
 }
}
</script>
<div class="thingToBeVotedOn" data-originalValue="6">
 <div class="count">6</div>
 <button class="up">Up</button>
 <button class="down">Down</button>
</div>

There are many ways to do this, the above is just an example that you can apply as you wish. You can add and remove classes to change the appearance of nodes based on what happens in side the if (increment) block.

I would think that clicking on "up" or "down" a second time should just be ignored, if a user wants to remove their up vote, they just click down and vice versa. Anyway, this works as you've asked.

The code can be more concise, but I prefer clarity over brevity. The original is awful from a code maintenace perspective.

• bob_cobb: Thanks for such a quick response. This looks 100x better than what I was currently doing. My primary goal is maintenance so I like how you abstracted this into one handle method. +1 on Math.abs() as well for keeping the integers positive. Seems to work pretty nicely. Offtopic but I think I found my new favorite stackexchange site. Much appreciated :)