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.