To-do list app using jQuery

Question

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

• Inkbug: You have leaking globals because of the semi-colon after name; (replace it with ,).
• delete me: Is this a homework assignment?
• Om3ga: @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.
• ANeves: @al0neevenings You forgot to share the HTML.
• delete me: @al0neevenings Ok, just asking because your wording reminded me of a homework assignment. I hope you find what you're looking for.

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