Looking for improvements on my jQuery-UI tagging widget

Question

I am looking for any possible improvements on this tagging widget.

I ask for special attention to be paid to my blur handler in the event of a autocomplete option being specified but any optimizations or critiques are welcome.

Demo:     http://webspirited.com/tagit  
Js File:  http://webspirited.com/tagit/js/tagit.js
Css File: http://webspirited.com/tagit/css/tagit.css 

Code:

(function($) {
    $.widget("ui.tagit", {

        // default options
        options: {
            tagSource:   [],
            triggerKeys: ['enter', 'space', 'comma', 'tab'],
            initialTags: [],
            minLength:   1
        },

        _keys: {
            backspace: 8,
            enter:     13,
            space:     32,
            comma:     44,
            tab:       9
        },

        //initialization function
        _create: function() {

            var self = this;
            this.tagsArray = [];

            //store reference to the ul
            this.element = this.element;

            //add class "tagit" for theming
            this.element.addClass("tagit");

            //add any initial tags added through html to the array
            this.element.children('li').each(function() {
                self.options.initialTags.push($(this).text());
            });

            //add the html input
            this.element.html('<li class="tagit-new"><input class="tagit-input" type="text" /></li>');

            this.input = this.element.find(".tagit-input");

            //setup click handler
            $(this.element).click(function(e) {
                if (e.target.tagName == 'A') {
                    // Removes a tag when the little 'x' is clicked.
                    $(e.target).parent().remove();
                    self._popTag();
                }
                else {
                    self.input.focus();
                }
            });

            //setup autcomplete handler
            this.options.appendTo = this.element;
            this.options.source = this.options.tagSource;
            this.options.select = function(event, ui) {
                self._removeTag();
                self._addTag(ui.item.value);
                return false;
            }
            this.input.autocomplete(this.options);

            //setup keydown handler
            this.input.keydown(function(e) {
                var lastLi = self.element.children(".tagit-choice:last");
                if (e.which == self._keys.backspace)
                    return self._backspace(lastLi);

                if (self._isInitKey(e.which)) {
                    e.preventDefault();
                    if ($(this).val().length >= self.options.minLength)
                        self._addTag($(this).val());
                }

                if (lastLi.hasClass('selected'))
                    lastLi.removeClass('selected');

                self.lastKey = e.which;
            });

            //setup blur handler
            this.input.blur(function(e) {
                self._addTag($(this).val());
                $(this).val('');
                return false;
            });

            //define missing trim function for strings
            String.prototype.trim = function() {
                return this.replace(/^\s+|\s+$/g, "");
            };

            this._initialTags();

        },
        _popTag: function() {
            return this.tagsArray.pop();
        }
        ,

        _addTag: function(value) {
            this.input.val("");
            value = value.replace(/,+$/, "");
            value = value.trim();
            if (value == "" || this._exists(value))
                return false;

            var tag = "";
            tag = '<li class="tagit-choice">' + value + '<a class="tagit-close">x</a></li>';
            $(tag).insertBefore(this.input.parent());
            this.input.val("");
            this.tagsArray.push(value);
        }
        ,

        _exists: function(value) {
            if (this.tagsArray.length == 0 || $.inArray(value, this.tagsArray) == -1)
                return false;
            return true;
        }
        ,

        _isInitKey : function(keyCode) {
            var keyName = "";
            for (var key in this._keys)
                if (this._keys[key] == keyCode)
                    keyName = key

            if ($.inArray(keyName, this.options.triggerKeys) != -1)
                return true;
            return false;
        }
        ,

        _removeTag: function() {
            this._popTag();
            this.element.children(".tagit-choice:last").remove();
        }
        ,

        _backspace: function(li) {
            if (this.input.val() == "") {
                // When backspace is pressed, the last tag is deleted.
                if (this.lastKey == this._keys.backspace) {
                    this._popTag();
                    li.remove();
                    this.lastKey = null;
                } else {
                    li.addClass('selected');
                    this.lastKey = this._keys.backspace;
                }
            }
            return true;
        }
        ,

        _initialTags: function() {
            if (this.options.initialTags.length != 0) {
                for (var i in this.options.initialTags)
                    if (!this._exists(this.options.initialTags[i]))
                        this._addTag(this.options.initialTags[i]);
            }
        }
        ,

        tags: function() {
            return this.tagsArray;
        }
        ,

        destroy: function() {
            $.Widget.prototype.destroy.apply(this, arguments); // default destroy
            this.tagsArray = [];
        }

    });
})(jQuery);

Answer 1

Cocky comment saying that this is almost max-optimized deserves a harsh review.

I don't know anything about the $.widget, from my brief look of it none of my comments can be ignored because it's "Something you need to do to play nicely with the $.widget".

Admittedly there are no obvious mistakes. Most of it is well structured and the code is readable. There are various nit picks all over the place though. Half of them can be ignored under the motto of that's a style specific comment.

Over all the code was pretty good.

Here are the main points:

Redundant:

this.element = this.element;

That's just plain redundant.

if (lastLi.hasClass('selected'))
     lastLi.removeClass('selected');

jQuery is clever enough to not remove a class that doesn't exist. The hasClass check is just as expensive as the removeClass call.

_popTag: function() {
    return this.tagsArray.pop();
}

That particular abstraction feels redundant when tagsArray is public aswell.

tags: function() {
    return this.tagsArray;
}

again it feels like a redundant abstractions when tagsArray is public. You do know tagsArray is public right?

HTML Strings versus DOM manipulation

this.element.html('<li class="tagit-new"><input class="tagit-input" type="text" /></li>');

As mentioned in the comments I don't like messing with HTML directly. Use DOM manipulation methods instead.

this.input = $(document.createElement("input"));
this.input.addClass("tagit-input");
this.input[0].type = "text";
var li = $("<li></li>");
li.addClass("tagit-new");
li.append(this.input);
this.element.empty();
this.element.append(li);

This can actaully be improved on with jQuery 1.4

this.input = $(document.createElement("input"), {
     "class": "tagit-input",
     "type": "text"
});
var li = $(document.createElement("li"), {
     "class": "tagit-new"
});
this.element.empty().append(li.append(this.input));

=== vs ==

if (e.target.tagName == 'A')

Always use === end of.

name duplication

this.options.appendTo = this.element;
this.options.source = this.options.tagSource;

Why do this? Why have the same thing mapped under different names? It's just hard to read and makes maintenance more of a pain. Also harder to refactor.

Return from a function or use if-else

if (e.which == self._keys.backspace)
     return self._backspace(lastLi);
if (self._isInitKey(e.which)) {

Might as well use else if here instead of returning. Avoid returning multiple times. There are some exceptions like guard statements or trivial functions.

Code smells

self.lastKey = e.which;

Storing the last key that was pressed? That smells. Refactor or abstract this away.

_removeTag: function() {
    this._popTag();
    this.element.children(".tagit-choice:last").remove();
}

You have both a remove and pop function. Refactor this so you only have one.

Caching

self._addTag($(this).val());
$(this).val('');

Really aught to cache it var $this = $(this). Get into this habit some calls to $ are really expensive.

Extending native prototypes

String.prototype.trim

Don't do it. Simple as. You'll break everyone else's code. The problem is code like this:

for (var c in "foo") {
     console.log(someString[c]); 
}
"f"
"o"
"o"
"function() { ... "

This is why we check for hasOwnProperty in for in loops. Personally I prefer to just assume no-one extends native prototypes and shout anyone that does that rather then use hasOwnProperty. Not to mention there's lots of 3rd party code you use that doesn't check for it.

if (x) return true

if (this.tagsArray.length == 0 || $.inArray(value, this.tagsArray) == -1)
     return false;
return true;

Ignoring the fact that if statements without brackets make me rage. This really aught to be

return this.tagsArray.length !== 0 && $.inArray(value, this.tagsArray) !== -1;

widget destroy function

destroy: function() {
    $.Widget.prototype.destroy.apply(this, arguments); // default destroy
    this.tagsArray = [];
}

The reason the widget makes call to .destroy is because there is no garbage collector for the DOM. Your supposed to clean up the dom. Your supposed to remove this.element, detach any event handlers. And whatever else needs cleaning up.

You do not need to clean up javascript. JS has a garbage collector.

Here is the full code annotated:

just ctrl-f //*

(function($) {
    $.widget("ui.tagit", {

        // default options
        options: {
            tagSource:   [],
            triggerKeys: ['enter', 'space', 'comma', 'tab'],
            initialTags: [],
            minLength:   1
        },

        _keys: {
            backspace: 8,
            enter:     13,
            space:     32,
            comma:     44,
            tab:       9
        },

        //initialization function
        _create: function() {

            var self = this;
            this.tagsArray = [];

            //store reference to the ul
            //* WTF? Seriously? you know theres only one this object right?
            this.element = this.element;

            //add class "tagit" for theming
            this.element.addClass("tagit");

            //add any initial tags added through html to the array
            this.element.children('li').each(function() {
                self.options.initialTags.push($(this).text());
            });

            //add the html input
            //* Style issue. i don't like passing html strings around.
            /*
                this.input = $("<input></input>");
                this.input.addClass("tagit-input");
                this.input[0].type = "text";
                var li = $("<li></li>");
                li.addClass("tagit-new");
                li.append(this.input);
                this.element.empty();
                this.element.append(li);
            */
            this.element.html('<li class="tagit-new"><input class="tagit-input" type="text" /></li>');

            this.input = this.element.find(".tagit-input");

            //setup click handler
            //*WTF this.element is already a jQuery object.
            $(this.element).click(function(e) {
                //*WTF use "==="
                if (e.target.tagName == 'A') {
                    // Removes a tag when the little 'x' is clicked.
                    //* Refactor this to ._popTag()
                    /*
                        self._popTag(e.target);
                    */
                    $(e.target).parent().remove();
                    self._popTag();
                }
                else {
                    self.input.focus();
                }
            });

            //setup autcomplete handler
            //*WTF Why map it to .appendTo. Feels like an annoying abstraction
            this.options.appendTo = this.element;
            //*WTF renaming it for no good reason
            this.options.source = this.options.tagSource;
            this.options.select = function(event, ui) {
                // Remove and add a tag? Feels like it could use a comment
                // or a swap/switch tags method
                self._removeTag();
                self._addTag(ui.item.value);
                return false;
            }
            this.input.autocomplete(this.options);

            //setup keydown handler
            this.input.keydown(function(e) {
                var lastLi = self.element.children(".tagit-choice:last");
                if (e.which == self._keys.backspace)
                    //*WTF don't return use else if statements.
                    return self._backspace(lastLi);

                if (self._isInitKey(e.which)) {
                    e.preventDefault();
                    //*WTF if the string in val is bigger then minLength?
                    // Too code specific give a high level explanation.
                    if ($(this).val().length >= self.options.minLength)
                        self._addTag($(this).val());
                }
                //*WTF Why are you removing the selected class from the last
                // li on every key down press? This really could use a comment.
                if (lastLi.hasClass('selected'))
                    lastLi.removeClass('selected');

                //*WTF storing the lastKey is a code smell. Reffactor this away
                self.lastKey = e.which;
            });

            //setup blur handler
            this.input.blur(function(e) {
                //*WTF always cache
                /*
                    var $this = $(this);
                */
                self._addTag($(this).val());
                //* WTF duplication. The input is already cleared in the
                // addtag.
                $(this).val('');
                return false;
            });

            //define missing trim function for strings
            //*WTF don't extend native prototypes. Your evil. Evil evil evil.
            // Theres a perfectly good $.trim. Use it!
            String.prototype.trim = function() {
                return this.replace(/^\s+|\s+$/g, "");
            };

            this._initialTags();

        },
        //*WTF Feels like a useless abstraction. _popTag is "internal" because
        // it starts with "_" but this.tagsArray is public. 
        _popTag: function() {
            return this.tagsArray.pop();
        }
        ,

        _addTag: function(value) {
            this.input.val("");
            value = value.replace(/,+$/, "");
            //*The extension! use var value = $.trim(value)
            value = value.trim();
            //* use "===" here. There's no reason why not to. 
            if (value == "" || this._exists(value))
                return false;

                var tag = "";
            //* Ew more string generated HTML. use the jquery or native DOM 
            // manipulation instead
            tag = '<li class="tagit-choice">' + value + '<a class="tagit-close">x</a></li>';
            $(tag).insertBefore(this.input.parent());
            //*WTF Sure. lets call it again? You know, just in case someone
            // managed to edit the input whilst where running IO blocking
            // javascript code.
            this.input.val("");
            //* Personally I would take the tagsArray and overwrite it's 
            // .push methods with the _addTag function. That way you can just
            // call this.tagsArray.push everywhere. Make sure to document it.
            // put it in the init function if you want.
            /*
                var _push = this.tagsArray.push;
                this.tagsArray.push = function(value) {
                    _push.apply(this, arguments);   
                    // do stuff here.
                };
            */
            this.tagsArray.push(value);
        }
        ,

        _exists: function(value) {
            //* check with === !!! Replace this with
            /*
                return this.tagsArray.length !== 0 &&
                    $.inArray(value, this.tagsArray) !== -1;
            */
            if (this.tagsArray.length == 0 || $.inArray(value, this.tagsArray) == -1)
                return false;
            return true;
        }
        ,

        _isInitKey : function(keyCode) {
            var keyName = "";
            for (var key in this._keys)
                //* Refactor this to 
                /*
                    if (this._keys[key] === keyCode) {
                        return $.inArray(key, this.options.triggerKeys) !== -1;
                    }
                */
                if (this._keys[key] == keyCode)
                    keyName = key//*WTF don't forget that semi colon.
            //* unneccesary if refactored.
            if ($.inArray(keyName, this.options.triggerKeys) != -1)
                return true;
            return false;
        }
        ,
        //*WTF It feels so wrong from a design point of view to have both
        // a popTag and a removeTag function. What's the difference?
        // Why do you remove it from the tagList but not from the DOM?
        _removeTag: function() {
            this._popTag();
            this.element.children(".tagit-choice:last").remove();
        }
        ,
        //*WTF the whole backspace in lastKey things means you backspace has 
        // two meanings when pressed byt eh user. It either selects it or
        // removes it. This just seems like bad design. Get rid of it.
        _backspace: function(li) {
            if (this.input.val() == "") {
                // When backspace is pressed, the last tag is deleted.
                if (this.lastKey == this._keys.backspace) {
                    //*WTF why not just call _removeTag!!!
                    this._popTag();
                    li.remove();
                    this.lastKey = null;
                } else {
                    li.addClass('selected');
                    this.lastKey = this._keys.backspace;
                }
            }
            return true;
        }
        ,

        _initialTags: function() {
            //*WTF use a > 0 check. checking for not 0 is just annoying,
            // Length can never be negative.
            if (this.options.initialTags.length != 0) {
                //*WTF avoid a for in loop on an array just use a standard
                /*
                    var len = this.options.initialTags.length;
                    for (var i=0;i < len;i++) {
                        var tag = this.options.initialTags[i];
                        if (!this._exists(tag)) {
                            this._addTag(tag);
                        }
                    }
                */
                for (var i in this.options.initialTags)
                    //*WTF you already check for existance in the addTag 
                    // function. Why do it here? Choose to do it either before
                    // calling or in the function not both.
                    if (!this._exists(this.options.initialTags[i]))
                        this._addTag(this.options.initialTags[i]);
            }
        }
        ,

        //*WTF why have both a tags & a tagsArray. Why? There both public.
        // It just a useless abstraction
        tags: function() {
            return this.tagsArray;
        }
        ,
        //*WTF What is this? C++. We have a garbage collector. if tagsArray
        // is no longer referenced anywhere then we don't need to null it on
        // purpose. 
        // From a $.widget point of view you want destroy to act as a garbage collector
        // for the DOM. Remove stuff from the DOM. The JS has it's own garbage
        // collector. 
        destroy: function() {
            $.Widget.prototype.destroy.apply(this, arguments); // default destroy
            this.tagsArray = [];
        }

    });
})(jQuery);