Как улучшить код, ооп

[meam]

http://jsfiddle.net/ovssubp8/12/
Что тут стоит улучшить? Что сделано не правильно и т.д. Надеюсь на вашу помощь

[nerv_]

в одном месте так, в другом сяк

this.elem.on('submit', '.todo-form', $.proxy(this.onFormSubmit, this));
//...
this.list.on('click', '.todo-x', this.onXClick.bind(this));

говорит о незнании того, что ты делаешь

onFormSubmit: function(e) {
if(this.input.val().trim() === '') {
*!*
    return false;
*/!*
}
*!*
    e.preventDefault();
*/!*
}

оригинально

function TaskList(el) {
    var that = this;
    // ...
*!*
    this.list.on('click', '.todo-x', this.onXClick.bind(this));
    this.el.on('click', '.todo-remove', this.onRemoveClick.bind(that))
*/!*
}

и т.д.

[meam]

this.elem.on('submit', '.todo-form', $.proxy(this.onFormSubmit, this));
//...
this.list.on('click', '.todo-x', this.onXClick.bind(this));

исправил

[nerv_]

Используешь метод trim стандарта ES5, но при этом используешь $.proxy...

Навешиваешь обработчик через addEventListener, но при этом используешь return false;

function TodoApp(opt) {
    // на один dom элемент навешиваешь события внутри TodoApp
    this.elem = opt.elem;
    this.taskList = new TaskList(this.elem);
    this.elem.on('submit', '.todo-form', $.proxy(this.onFormSubmit, this))
}

function TaskList(el) {
    var that = this;
    this.el = el;
    // и внутри TaskList
    this.el.on('click', '.todo-remove', $.proxy(this.onRemoveClick, this))
}

Беда.

Кто-то не знает, как передавать контекст в итерирующие методы...

var that = this;
        var oldTasks = that.tasks;
        that.tasks = [];
        oldTasks.forEach(function(e) {
            if(e.done) {
                $(e.li).remove();
            }else {
                that.tasks.push(e);
            }
        })

и т.д.

[WorM32]

// jsDOC
function TodoApp(opt) {// лучше использовать полное options
    this.elem = opt.elem;
    this.taskList = new TaskList(this.elem);
    this.input = this.elem.find('.todo-name'); 
    this.elem.on('submit', '.todo-form', $.proxy(this.onFormSubmit, this)) //пропущена ;. Почему не просто this.onFormSubmit.bind(this)?

}
TodoApp.prototype = {
    constructor: TodoApp,
    onFormSubmit: function() { // параметр e
        if(this.input.val().trim() === '') { // this.input.val() в переменную, тк используется 2 раза.
            return false; // нет! только e.preventDefault. return false аналогично вызову e.preventDefault() и e.stopPropagation(), использовать stopPropagation плохой тон.
        }
        this.taskList.addTask(this.input.val());
        this.input.val('');
        return false;
    }
};
function TaskList(el) {
    var that = this;
    this.el = el; // el, elem - определись уже. к тому же el - это jQuery переменная, jQuery переменные лучше обозначать используя префикс $ (например, $el)
    this.list = el.find('.todo-list');
    this.tasks = [];
    this.list.on('dblclick', '.todo-task', function(e) { // неправильный подход. TaskList должен содержать коллекцию Task и оперерировать уже ими. Так что dbclick должен слушать и обарабывать Task.
        var task = $(e.currentTarget).data('task');
        var newName = prompt('', task.name);
        task.edit(newName);
    });
    this.list.on('change', '.todo-check', function(e) {
        var task = $(e.currentTarget).parent().data('task'); // parent() - зло
        task.onCheckChange();
    });
    this.list.on('click', '.todo-x', $.proxy(this.onXClick, this)); // это должно быть у Task во вьюхе
    this.el.on('click', '.todo-remove', $.proxy(this.onRemoveClick, this)) // ;

}
TaskList.prototype = {
    constructor: TaskList,
    addTask: function(name) {
        var li = $('<li/>').addClass('todo-task animated flash'); // неее. Должен быть TaskView, который будет этой хренью заниматься. А здесь должн создаваться модель Task и на основе модели TaskView
        this.list.append(li);
        var task = new Task(li, name);
        this.tasks.push(task);
        li.data('task', task); // не понятно, зачем засовывать в this.task и в data элемента. косяк
        console.log(this.tasks);
    },
    onXClick: function(e) {
        var task = $(e.currentTarget).parent('li').data('task');
        task.li.remove();
        this.tasks.forEach(function(e,i,a) {
            if(e === task) {
                a.splice(i,1)
            }
        })
    },
    onRemoveClick: function() {
        var that = this; // лишнее
        var oldTasks = that.tasks;
        that.tasks = [];
        oldTasks.forEach(function(e) { // можно использовать bind(this), либо передать конекст 3-им аргументом
            if(e.done) {
                $(e.li).remove(); // ну и опять же, здесь должно быть вызов модели Task
            }else {
                that.tasks.push(e);
            }
        })
    }
};
function Task(li ,name) {
    this.li = li;
    this.name = name;
    this.done = false;
    this.renderTask();
}
Task.prototype = {
    constructor: Task,
    renderTask: function() {
        this.li.html('<input type="checkbox" class="todo-check"> ' +this.name+ '<span class="todo-x"> X </span>');

    },
    edit: function(newName) {
        if(this.done) {
            this.done = false;
            this.li.removeClass('done') // ;
        }
        this.name = newName;
        this.renderTask();
    },
    onCheckChange: function() {
        this.li.toggleClass('done');
        this.done = !this.done;
    }

};
new TodoApp({
    elem: $('.todo') // ;
});

Вторую половину смотрел уже слабо, устал)

[melky]

new TodoApp({
    elem: $('.todo')
});

скрипт может быть вызван до загрузки дерева, а не после.

для того, чтобы уйти от этого, нужно вынести инициализацию в $(function () { /*КОД*/ });
и даже не в неё, а в :

jQuery(function ($) { /*КОД*/ });

потому что $ - это не всегда jQuery

[melky]

this.list.on('dblclick', '.todo-task', function(e) {
        var task = $(e.currentTarget).data('task');
        var newName = prompt('', task.name);
        task.edit(newName);
    });
    this.list.on('change', '.todo-check', function(e) {
        var task = $(e.currentTarget).parent().data('task');
        task.onCheckChange();
    });
    this.list.on('click', '.todo-x', $.proxy(this.onXClick, this));

обычно навешивание делается в один вызов. для этого передаётся карта имен событий к обработчикам. чё по чём - в документации показано.

... вообще, что делают обработчики событий в конструкторе? нужно вынести их в инициализацию, или что-то в этом роде (document.ready)

[melky]

function TaskList(el) {
    var that = this;
//...

так лучше не делать. конструктор легко может стать замыканием, а это очень плохо - при создании ~1000 объектов

[melky]

Phil Karlton: «There are only two hard things in Computer Science: cache invalidation and naming things.»
(В программировании две беды: инвалидация кеша и именование переменных)

поговорим про вторую беду:

onXClick: function(e) {
        //..
    },
    onRemoveClick: function() {
        //..
    }

плохие имена для методов. в имени сущности (любой) должно быть показано, для чего она.

не глядя в код, сможешь рассказать, что внутри этих методов и зачем они? а для этих?

function SmileList () { }
SmileList.prototype = {
    // да, в классе 2 метода.
    // и если что-то сломается, то
    // для поиска бага в коде придется читать содержимое этих методов. 
    // и ЭТО НЕ ВЕСЕЛО, когда кода очень много.
    // и писал его не ты, а кто-то другой.
    click: function(e) {
        //..
    },
    mousedown: function() {
        //..
    }
};

для сравнения, это имя - получше, но тоже не то.

addTask: function(name) {
        //...
    },

ещё пример:

Task.prototype = {
    constructor: Task,
    renderTask: function() {
       /// ....
    },
    edit: function(newName) {
        //....
    },
    onCheckChange: function() {
        //...
    }

};

Task.renderTask - зачем постфикс (красный)?
Task.edit - неплохо

поподробнее про TaskList.addTask :

TaskList.addTask

какие сущности могут быть внутри TaskList ? ничего, кроме TASK. постфикс "Task" для addTask не нужен.

и, судя по названию класса, это упорядоченный список (List).

операция ДОБАВИТЬ (add) больше подходит для списков, где порядок не важен

пример такого списка - список классов HTML

var li = $('<li/>').*!*addClass*/!*('todo-task animated flash');

а там, где порядок важен, обычно именуют так:

this.list.*!*append*/!*(li);
this.tasks.*!*push*/!*(task)

append, push - отлично.


т.е. метод переименован из TaskList.addTask в TaskList.append.

вообще, имена для переменных - вопрос вкуса, конечно, но лучше именовать их так, чтобы было понятно о методе\переменной без залезания вовнутрь неё

[WorM32]

melky,
ничего не вижу плохого в наименовании обработчиков событий

onXClick: function(e) {
        //..
    },
    onRemoveClick: function() {
        //..
    }

Сам использую такой же подход. Из названия сразу видно onЭлементClick, что это:

• Обработчик события (префикс on). При беглом просмотре большого куска кода, сразу видно, какой метод связан с событиями, а какой нет.
• Элемент взаимодействия (Элемент)
• Название события (суффикс Click)