patternjavascriptMinor
Simple todo list
Viewed 0 times
listtodosimple
Problem
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);
}Solution
-
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.
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.
-
Limit accessing the DOM. Within your
-
Combine code into functions to simplify. You are creating a delete link within the
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.
-
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.
-
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):
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();-
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);
...
}
}-
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.
-
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.
-
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();Code Snippets
(function() {
... // all your code goes here.
})();function toDoList() {
... // code
}
toDoList();function toDoList() {
var list = document.getElementById('todoList');
var addButton = document.getElementById('btnAdd');
...
function addItem() {
...
list.appendChild(listElement);
...
}
}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());
...
}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();Context
StackExchange Code Review Q#85116, answer score: 5
Revisions (0)
No revisions yet.