Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.

Thanks in advance.

Here is my code.

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

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

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

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

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

here is my todo.js file

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

    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>");
            contents.prepend(div);
            $('.name').val('').focus();
        }
        return;
    },

    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
    };
};
share|improve this question
2  
You have leaking globals because of the semi-colon after name; (replace it with ,). –  Inkbug Aug 9 '12 at 12:43
 
Is this a homework assignment? –  st-boost Aug 10 '12 at 3:12
 
@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. –  al0ne evenings Aug 10 '12 at 6:05
 
@al0neevenings You forgot to share the HTML. –  ANeves Aug 10 '12 at 16:32
 
@al0neevenings Ok, just asking because your wording reminded me of a homework assignment. I hope you find what you're looking for. –  st-boost Aug 11 '12 at 3:23

1 Answer

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) {
            e.preventDefault();
            e.stopPropagation();
            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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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