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: