I created a simple todo kind of app using JavaScript. It is working fine but the code which I wrote is not proper. Please review my code and improve it if it is needed.

$(function () {
    var todo = new Todo('contents');

    $('.addBtn').on('click', function() {
        var name = $(this).parent().find('input[type="text"]').val();

    $('.contents').on('click', '.remove', function() {
        var el = $(this).parent();

    $('.contents').on('click', '.update', function() {
        var dom = $(this);

    $('.contents').on('click', '.updateBtn', function() {
        var el = $(this);

Here is my todo.js file

var Todo = function(c) {
    var contents = $('.' + c),

    add = function (name) {
        if(name != "") {
            var div = $('<div class="names"></div>');
            div.append('<span>' + name + '</span>');
            div.append("<button class='update' class='update'>Edit</button>");
            div.append("<button class='remove' name='remove'>Remove</button>");

    addUpdateField = function (dom) {
        var name = dom.parent().find('span').text(),
            field = $('<input type="text" value="' + name + '" />'),
            update = $('<button class="updateBtn">Update</button>');

        return dom.parent().html('').append(field).append(update);

    update = function(el) {
        var val = el.parent().find('input').val();
        el.parent().html('<span>' + val + '</span>')
        .append('<button class="update" class="update">Edit</button>')
        .append('<button class="remove" class="remove">Remove</button>');

    remove = function (el) {
        return el.remove();

    return {
        add             : add,
        update          : update,
        remove          : remove,
        addUpdateField  : addUpdateField
  • 2
    \$\begingroup\$ You have leaking globals because of the semi-colon after name; (replace it with ,). \$\endgroup\$
    – Inkbug
    Commented Aug 9, 2012 at 12:43
  • \$\begingroup\$ Is this a homework assignment? \$\endgroup\$
    – delete me
    Commented Aug 10, 2012 at 3:12
  • \$\begingroup\$ @st-boost No it is not homework assignment. I am learning modular javascript and want to improve my coding. Since here are professional javascript developer on this forum so I thought this would be the best place for me to ask for improvement in my code. \$\endgroup\$
    – Om3ga
    Commented Aug 10, 2012 at 6:05
  • \$\begingroup\$ @al0neevenings You forgot to share the HTML. \$\endgroup\$
    – ANeves
    Commented Aug 10, 2012 at 16:32
  • \$\begingroup\$ @al0neevenings Ok, just asking because your wording reminded me of a homework assignment. I hope you find what you're looking for. \$\endgroup\$
    – delete me
    Commented Aug 11, 2012 at 3:23

1 Answer 1


As I read it, you want:

  • one <div class="names"> per todo (in this case inside .contents) surrounding the todo text in a span and the buttons "Edit" / "Remove"
  • on clicking "Edit", the span is exchanged for an input field and an "Update" button, "Edit" and "Remove" are removed.
  • on clicking "Update", the prior state with the new text is recreated (span, Edit, Remove)

After you follow Inkbug`s advice from his comment, you should probably do this:

  • cache the todo root element (var todoRoot = $(...);)
  • rename addUpdateField/.update to edit/.edit and updateBtn to update - consistent with the button texts and easier to read
  • change the api of your todo.js functions. For update / edit / remove, just use the dom node of the div as the argument:

    function(target, action) {
        todoRoot.on('click', target, function(e) {
            action.call(todo, $(this).parents('.names').first());
    bindClick('.remove', todo.remove);
    bindClick('.edit',   todo.edit);
    bindClick('div',     todo.update);
  • don't use that many appends. Put it all in a string (that can contain more than one DOM node!)

  • don't create / delete all the nodes. Create all three buttons, use .toggleClass(...) and CSS to make all buttons/fields disappear that you don't need. That moves all your HTML into the add function and goes easy on the DOM
  • move the bindings into the Todo function. You don't want to have to remember to bind these events for every Todo list you create

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.