Искала улучшений на мой jQuery-интерфейс виджета тегов


Я ищу любые возможные улучшения на этой виджета тегов.

Я прошу уделить особое внимание к моей размытие обработчик в случае автозаполнения вариант уточняется, но каких-либо оптимизаций или критика приветствуются.

Демо: http://webspirited.com/tagit
JS файл: http://webspirited.com/tagit/js/tagit.js
CSS-файл: http://webspirited.com/tagit/css/tagit.css

Код:

(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);


3242
8
задан 17 февраля 2011 в 09:02 Источник Поделиться
Комментарии
1 ответ

Дерзкий комментарий, сказав, что это почти максимум-оптимизирован заслуживает сурового рассмотрения.

Я не знаю ничего про $.виджет, на мой беглый взгляд это не мои комментарии могут быть проигнорированы, потому что это "что-то, что вам нужно сделать, чтобы играть красиво с $.виджет".

Правда, нет никаких очевидных ошибок. Большинство из них хорошо структурирована и код легко читается. Существуют различные гнида за все, хотя место выбирает. Половина из них могут быть проигнорированы под девизом, что это определенный стиль комментария.

За весь код был довольно хорошим.

Вот несколько основных пунктов:

Избыточные:

это.элемент = это.элемента;

Это просто излишним.

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

jQuery-это достаточно умен, чтобы не удалить класс, который не существует. Проверка hasClass так же дорого, как removeClass звонок.

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

Что конкретное абстракция чувствует себя лишним, когда tagsArray является публичным, а также.

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

снова он чувствует, как ненужных абстракций, когда tagsArray является публичной. Вы знаете tagsArray это право?

HTML-строк против манипуляций с DOM

this.element.html('

  • ');

    Как упоминалось в комментариях, я не люблю возиться с HTML напрямую. Вместо того, чтобы использовать методы манипуляции DOM.

    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);

    Это может actaully быть улучшены с помощью 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));

    === против ==

    если (электронная.цель.у tagName == 'а')

    Всегда использовать === конец.

    дублирование имени

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

    Зачем это делать? Почему же альбом под разными именами? Это просто трудно читать и делает техническое обслуживание более боли. Также сложнее рефакторинга.

    Возврат из функции или использовать если-нибудь

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

    Можно также использовать еще, если здесь вместо возвращения. Во избежание возвращения несколько раз. Есть некоторые исключения, как охранник высказываний или тривиальных функций.

    Код попахивает

    самовывоз.lastKey = е.который;

    Хранение последнего клавишу, которая была нажата? Что пахнет. Рефакторинг или реферат это.

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

    У вас нет как снять и поп-функцию. Рефакторинг это так, у вас есть только один.

    Кэширование

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

    Действительно, чтобы кэшировать его ВАР $это = $(это). Попасть в эту привычку несколько вызовов $ очень дорого.

    Расширение собственных прототипов

    Строку.прототип.отделка

    Не делай этого. Простой как. Вы сломаете код всех остальных. Проблема заключается в код такой:

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

    Вот почему мы проверяем, метод hasOwnProperty в для петли. Лично я предпочитаю просто считать никто не расширяет прототипы встроенных объектов и кричать Кто что делает, а затем использовать метод hasOwnProperty. Много не говоря уже о 3-й партии код, который вы используете, не проверить его.

    если (Х) возвращает true

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

    Игнорируя тот факт, что если заявления без скобок заставить меня ярость. Это действительно ничего, чтобы быть

    вернуть это.tagsArray.длина !== 0 && $.inArray(значение, это.tagsArray) !== -1;

    виджет функции destroy

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

    Почему виджет делает вызов .уничтожить именно потому, что нет сборщика мусора на дом. Ваш должен убирать дом. Ваш должен удалить это.элемент, отсоедините все обработчики событий. И все остальное, нуждается в очистке.

    Вам не нужно убирать на JavaScript. JS имеет сборщик мусора.

    Вот полный код аннотациями:

    просто Ctrl-Ф //*

    (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);

    5
    ответ дан 19 февраля 2011 в 06:02 Источник Поделиться