Simple todo list

Question

Can someone tell me how good/bad this code is for a super simple todo list made from vanilla JavaScript? Anything to work on or consider on my next projects?

// establish variables
var addButton = document.getElementById('btnAdd');
// event listeners
addButton.addEventListener("click", addItem);
// assign event listener to all delete links
function assignDeleteLinkEvent(){
 var deleteLinks = document.getElementsByClassName('delete-link');
 for (var i=0;i<deleteLinks.length;i++){
 deleteLinks[i].addEventListener("click", removeItem);
 }
}
function addItem(){
 // establish variables
 var textBox = document.getElementById('textBox');
 var list = document.getElementById('todoList');
 var listElement = document.createElement('li');
 var deleteLink = document.createElement('a');
 // append item to list
 list.appendChild(listElement);
 listElement.innerText = textBox.value;
 // append delete link to item
 deleteLink.setAttribute("href", "#");
 deleteLink.setAttribute("class", "delete-link");
 deleteLink.innerHTML = "x";
 listElement.appendChild(deleteLink);
 assignDeleteLinkEvent();
 // reset input box
 textBox.value = "";
}
// removes the item
function removeItem(){
 var parent = this.parentNode.parentNode;
 var child = this.parentNode
 parent.removeChild(child);
}

Answer 1

1. Don't pollute the global namespace. You have 1 variable and 3 functions all going into the global namespace. This is especially a problem because of the generic names you have. So they could be overwritten without you knowing it and it will cause issues. The easiest way to fix this is to wrap all your code in a self executing function.

   (function() {
    ... // all your code goes here.
   })();
   

   For your purposes, that would work. Another option is to wrap it in a function and call that function. This would allow you to supply parameters if needed later on.

   function toDoList() {
    ... // code
   } 
   toDoList();
   

2. Limit accessing the DOM. Within your addItem function you are requerying the DOM for the todoList and the textbox everytime. These won't change so these variables could be set outside of this function.

   function toDoList() {
    var list = document.getElementById('todoList');
    var addButton = document.getElementById('btnAdd');
    ...
    function addItem() {
    ...
    list.appendChild(listElement);
    ...
    }
   }
   

3. Combine code into functions to simplify. You are creating a delete link within the addItem function. This involves 4 steps. Move all of this into a separate function and call that. This makes your addItem function simpler and centralizes the delete link creation code.

   function createDeleteLink() {
    var deleteLink = document.createElement('a');
    deleteLink.setAttribute("href", "#");
    deleteLink.setAttribute("class", "delete-link");
    deleteLink.innerHTML = "x";
    return deleteLink
   }
   function addItem() {
    ...
    listElement.appendChild(createDeleteLink());
    ...
   }
   

   Now that the creation of the delete link is by itself it makes it easier to see that we can just add the click event listener directly on creation instead of looping through every link every time (which was actually adding listeners to items that already had listeners.

4. Unnecessary comments. Comments such as "Removes an item" right above a function that is called "removeItem" is useless. In fact it will just be a distraction. Limit comments to describe tricky code or anything that might be confusing later.

5. Future considerations. If you want to re-use this code you might consider an options object as a parameter to the main function. This could include things such as the id of the main list, the text for the delete link, class names, etc... This would allow you to use this same code on multiple pages and also allow for customization.

Following my ideas above (except #5):

 function initToDoList() {
 var textBox = document.getElementById('textBox');
 var list = document.getElementById('todoList');
 var addButton = document.getElementById('btnAdd');
 addButton.addEventListener("click", addItem);
 function createDeleteLink() {
 var deleteLink = document.createElement('a');
 deleteLink.setAttribute("href", "#");
 deleteLink.setAttribute("class", "delete-link");
 deleteLink.innerHTML = "x";
 deleteLink.addEventListener("click", removeItem);
 return deleteLink;
 }
 function addItem() {
 var listElement = document.createElement('li');
 listElement.innerText = textBox.value;
 listElement.appendChild(createDeleteLink());
 list.appendChild(listElement);
 textBox.value = "";
 }
 function removeItem() {
 var parent = this.parentNode.parentNode;
 var child = this.parentNode
 parent.removeChild(child);
 }
 }
 initToDoList();

• Yeysides: awesome! one question though, is there a naming convention for the object passed into the main function if I decide to do that?
• nickles80: I would use "options". And it should be an object. In your function you establish defaults to all needed options and then override any from the incoming object. So the consumer can override as many or as few as they need. They discuss this here: stackoverflow.com/questions/9602449/…
• numerek: you still add initToDoList into whatever context it is created in possibly global, you could do this instead (function () { /* initToDoList code here */ })()