jQuery Upvote, a simple Stack Exchange style voting plugin

Question

I'm not very good at JavaScript, so it would be great if somebody could review my jQuery Upvote plugin, and point out mistakes, bad practices, or anything suspicious.

The repository is on GitHub. Here's a live demo, and qunit tests, if you're interested, but I'm not asking for a review of those.

Here's the code, with the header omitted (full version here):

;(function($) {
    "use strict";
    var namespace = 'upvote';
    var dot_namespace = '.' + namespace;
    var upvote_css = 'upvote';
    var dot_upvote_css = '.' + upvote_css;
    var upvoted_css = 'upvote-on';
    var dot_upvoted_css = '.' + upvoted_css;
    var downvote_css = 'downvote';
    var dot_downvote_css = '.' + downvote_css;
    var downvoted_css = 'downvote-on';
    var dot_downvoted_css = '.' + downvoted_css;
    var star_css = 'star';
    var dot_star_css = '.' + star_css;
    var starred_css = 'star-on';
    var dot_starred_css = '.' + starred_css;
    var count_css = 'count';
    var dot_count_css = '.' + count_css;
    var enabled_css = 'upvote-enabled';

    function init(options) {
        return this.each(function() {
            methods.destroy.call(this);

            var count = parseInt($(this).find(dot_count_css).text());
            count = isNaN(count) ? 0 : count;
            var initial = {
                id: $(this).attr('data-id'),
                count: count,
                upvoted: $(this).find(dot_upvoted_css).size(),
                downvoted: $(this).find(dot_downvoted_css).size(),
                starred: $(this).find(dot_starred_css).size(),
                callback: function() {}
            };

            var data = $.extend(initial, options);
            if (data.upvoted && data.downvoted) {
                data.downvoted = false;
            }

            var that = $(this);
            that.data(namespace, data);
            render(that);
            setupUI(that);
        });
    }

    function setupUI(that) {
        that.find(dot_upvote_css).addClass(enabled_css);
        that.find(dot_downvote_css).addClass(enabled_css);
        that.find(dot_star_css).addClass(enabled_css);
        that.find(dot_upvote_css).on('click.' + namespace, function() {
            that.upvote('upvote');
        });
        that.find('.downvote').on('click.' + namespace, function() {
            that.upvote('downvote');
        });
        that.find('.star').on('click.' + namespace, function() {
            that.upvote('star');
        });
    }

    function _click_upvote() {
        this.find(dot_upvote_css).click();
    }

    function _click_downvote() {
        this.find(dot_downvote_css).click();
    }

    function _click_star() {
        this.find(dot_star_css).click();
    }

    function render(that) {
        var data = that.data(namespace);
        that.find(dot_count_css).text(data.count);
        if (data.upvoted) {
            that.find(dot_upvote_css).addClass(upvoted_css);
            that.find(dot_downvote_css).removeClass(downvoted_css);
        } else if (data.downvoted) {
            that.find(dot_upvote_css).removeClass(upvoted_css);
            that.find(dot_downvote_css).addClass(downvoted_css);
        } else {
            that.find(dot_upvote_css).removeClass(upvoted_css);
            that.find(dot_downvote_css).removeClass(downvoted_css);
        }
        if (data.starred) {
            that.find(dot_star_css).addClass(starred_css);
        } else {
            that.find(dot_star_css).removeClass(starred_css);
        }
    }

    function callback(that) {
        var data = that.data(namespace);
        data.callback(data);
    }

    function upvote() {
        var data = this.data(namespace);
        if (data.upvoted) {
            data.upvoted = false;
            --data.count;
        } else {
            data.upvoted = true;
            ++data.count;
            if (data.downvoted) {
                data.downvoted = false;
                ++data.count;
            }
        }
        render(this);
        callback(this);
        return this;
    }

    function downvote() {
        var data = this.data(namespace);
        if (data.downvoted) {
            data.downvoted = false;
            ++data.count;
        } else {
            data.downvoted = true;
            --data.count;
            if (data.upvoted) {
                data.upvoted = false;
                --data.count;
            }
        }
        render(this);
        callback(this);
        return this;
    }

    function star() {
        var data = this.data(namespace);
        data.starred = ! data.starred;
        render(this);
        callback(this);
        return this;
    }

    function count() {
        return this.data(namespace).count;
    }

    function upvoted() {
        return this.data(namespace).upvoted;
    }

    function downvoted() {
        return this.data(namespace).downvoted;
    }

    function starred() {
        return this.data(namespace).starred;
    }

    var methods = {
        init: init,
        count: count,
        upvote: upvote,
        upvoted: upvoted,
        downvote: downvote,
        downvoted: downvoted,
        starred: starred,
        star: star,
        _click_upvote: _click_upvote,
        _click_downvote: _click_downvote,
        _click_star: _click_star,
        destroy: destroy
    };

    function destroy() {
        return $(this).each(function() {
            $(window).unbind(dot_namespace);
            $(this).removeClass(enabled_css);
            $(this).removeData(namespace);
        });
    }

    $.fn.upvote = function(method) {  
        if (methods[method]) {
            return methods[method].apply(this, Array.prototype.slice.call(arguments, 1));
        }
        if (typeof method === 'object' || ! method) {
            return methods.init.apply(this, arguments);
        }
        $.error('Method ' + method + ' does not exist on jQuery.upvote');
    };  
})(jQuery);

I pasted the code into jshint.com, and it gives 21 warnings: "Possible strict violation", for most but not all places where I use this. What does that mean, and how to improve it?

Answer 1

This in JSHint

JSHint says "Possible strict violation" because you are using this inside something that, as far as it can tell, is not a method.

In non-strict mode, calling _click_downvote(5) would bind this to the global object (window in the browser). In strict mode, this would be undefined, and you would get in trouble. Binding it as a listener in jQuery would make this the target of the event.

If that's how you intend to implement, you can ignore JSHint, as you will not generate any errors. But, it is telling you that your code is unclear to anyone reading it, because using this inside of something that is not obviously a method is quite confusing. It would be better to simply pass the object as a parameter.

See the explanation in this SO Answer.

Also your missing a radix parameter in parseInt in line 25 presumably you want the number in base 10, so the line should be:

            var count = parseInt($(this).find(dot_count_css).text(),10);

It'll work fine either way, but you said 'use strict'. :)

Answer 2

I like your code, I wish I could write code like that in languages I'm not very good at ;)

The Possible strict violation stems from the fact that your function could easily be called without setting this and then this would point to window and basically you could then be setting global variables without using var which is a strict violation.

You could solve this by either placing your functions within an object with Literal Notation or by provide this as a a parameter. I would go for Literal Notation. Some bedtime reading.

Some minor items;

• var that = $(this); -> I would go for var $this = $(this);, that is usually indicating that you are going to use a closure.

Answer 3

The answers helped:

• I fixed the parseInt call to use a radix
• I understood that var that = $(this); is NOT a good name
• I understood that I'm using this incorrectly

From conversations with @Dagg I also understood that:

• $this is not a good name either, as this itself is already unclear what it is, and sticking a $ which effectively makes it look like a Hungarian notation for "jQuery this", which makes no sense at all
• I understood that I don't understand sqat about this, and what kind of values it takes on in the code above
• I understood that I should avoid using this as much as possible

Although I understood many things that were wrong, the solution was still far from clear. So I dug deeper. The most important piece was really understandig what value this will take in the method calls where JSHint was reporting a warning. And the fix is to NOT use this at those places, but to pass in an object, to make it perfectly clear what the method is working with.

As for the naming of var something = $(this), I went with jqdom, though I realize it's not great.

I rewrote it like this, solving all JSHint warnings:

function init(dom, options) {
    return dom.each(function() {
        var jqdom = $(this);
        methods.destroy(jqdom);

        var count = parseInt(jqdom.find(dot_count_css).text(), 10);
        count = isNaN(count) ? 0 : count;
        var initial = {
            id: jqdom.attr('data-id'),
            count: count,
            upvoted: jqdom.find(dot_upvoted_css).size(),
            downvoted: jqdom.find(dot_downvoted_css).size(),
            starred: jqdom.find(dot_starred_css).size(),
            callback: function() {}
        };

        var data = $.extend(initial, options);
        if (data.upvoted && data.downvoted) {
            data.downvoted = false;
        }

        jqdom.data(namespace, data);
        render(jqdom);
        setupUI(jqdom);
    });
}

function setupUI(jqdom) {
    jqdom.find(dot_upvote_css).addClass(enabled_css);
    jqdom.find(dot_downvote_css).addClass(enabled_css);
    jqdom.find(dot_star_css).addClass(enabled_css);
    jqdom.find(dot_upvote_css).on('click.' + namespace, function() {
        jqdom.upvote('upvote');
    });
    jqdom.find('.downvote').on('click.' + namespace, function() {
        jqdom.upvote('downvote');
    });
    jqdom.find('.star').on('click.' + namespace, function() {
        jqdom.upvote('star');
    });
}

function _click_upvote(jqdom) {
    jqdom.find(dot_upvote_css).click();
}

function _click_downvote(jqdom) {
    jqdom.find(dot_downvote_css).click();
}

function _click_star(jqdom) {
    jqdom.find(dot_star_css).click();
}

function render(jqdom) {
    var data = jqdom.data(namespace);
    jqdom.find(dot_count_css).text(data.count);
    if (data.upvoted) {
        jqdom.find(dot_upvote_css).addClass(upvoted_css);
        jqdom.find(dot_downvote_css).removeClass(downvoted_css);
    } else if (data.downvoted) {
        jqdom.find(dot_upvote_css).removeClass(upvoted_css);
        jqdom.find(dot_downvote_css).addClass(downvoted_css);
    } else {
        jqdom.find(dot_upvote_css).removeClass(upvoted_css);
        jqdom.find(dot_downvote_css).removeClass(downvoted_css);
    }
    if (data.starred) {
        jqdom.find(dot_star_css).addClass(starred_css);
    } else {
        jqdom.find(dot_star_css).removeClass(starred_css);
    }
}

function callback(jqdom) {
    var data = jqdom.data(namespace);
    data.callback(data);
}

function upvote(jqdom) {
    var data = jqdom.data(namespace);
    if (data.upvoted) {
        data.upvoted = false;
        --data.count;
    } else {
        data.upvoted = true;
        ++data.count;
        if (data.downvoted) {
            data.downvoted = false;
            ++data.count;
        }
    }
    render(jqdom);
    callback(jqdom);
    return jqdom;
}

function downvote(jqdom) {
    var data = jqdom.data(namespace);
    if (data.downvoted) {
        data.downvoted = false;
        ++data.count;
    } else {
        data.downvoted = true;
        --data.count;
        if (data.upvoted) {
            data.upvoted = false;
            --data.count;
        }
    }
    render(jqdom);
    callback(jqdom);
    return jqdom;
}

function star(jqdom) {
    var data = jqdom.data(namespace);
    data.starred = ! data.starred;
    render(jqdom);
    callback(jqdom);
    return jqdom;
}

function count(jqdom) {
    return jqdom.data(namespace).count;
}

function upvoted(jqdom) {
    return jqdom.data(namespace).upvoted;
}

function downvoted(jqdom) {
    return jqdom.data(namespace).downvoted;
}

function starred(jqdom) {
    return jqdom.data(namespace).starred;
}

var methods = {
    init: init,
    count: count,
    upvote: upvote,
    upvoted: upvoted,
    downvote: downvote,
    downvoted: downvoted,
    starred: starred,
    star: star,
    _click_upvote: _click_upvote,
    _click_downvote: _click_downvote,
    _click_star: _click_star,
    destroy: destroy
};

function destroy(jqdom) {
    return jqdom.each(function() {
        $(window).unbind(dot_namespace);
        $(this).removeClass(enabled_css);
        $(this).removeData(namespace);
    });
}

$.fn.upvote = function(method) {  
    var args;
    if (methods[method]) {
        args = Array.prototype.slice.call(arguments, 1);
        args.unshift(this);
        return methods[method].apply(this, args);
    }
    if (typeof method === 'object' || ! method) {
        args = Array.prototype.slice.call(arguments);
        args.unshift(this);
        return methods.init.apply(this, args);
    }
    $.error('Method ' + method + ' does not exist on jQuery.upvote');
};