Todo App with JavaScript

Question 1

I am learning JavaScript and this is a simple todo application I made which uses local storage. It is currently running at: https://koraytugay.github.io/vanillatodo/#. The code is as follows. I am open to any suggestions on how the structure / architecture or implementation details should/can be improved. Can be cloned from: https://github.com/koraytugay/vanillatodo and it looks like this:

screenshot

index.html

<!DOCTYPE html>
<html>
<head>
 <title>VanillaTodo</title>
 <link rel="stylesheet" href="style.css"/>
</head>
<body>
<div id="container">
 <input id="new-todo-input" type="text">
 <h2>To Do Items</h2>
 <div id="todo-items"></div>
 <hr/>
 <a href="#" id="delete-all-todo-items">Delete All</a>
</div>
<script type="module" src="src/todoController.js"></script>
</body>
</html>

todoItemStorageService.js

export default (function() {
 const todoItems = 'todo-items';
 const idCounter = 'id-counter';
 function getTodoItems() {
 if (localStorage.getItem(todoItems) === null) {
 localStorage.setItem(todoItems, JSON.stringify([]));
 localStorage.setItem(idCounter, '1');
 }
 return JSON.parse(localStorage.getItem(todoItems));
 }
 function getNextTodoItemId() {
 const nextTodoItemId = parseInt(localStorage.getItem(idCounter));
 localStorage.setItem(idCounter, '' + (nextTodoItemId + 1));
 return nextTodoItemId;
 }
 function setTodoItems(items) {
 localStorage.setItem(todoItems, JSON.stringify(items));
 }
 function deleteTodoItem(id) {
 setTodoItems(getTodoItems().filter(todoItem => todoItem.id !== id));
 }
 function deleteTodoItems() {
 setTodoItems([]);
 }
 function addTodoItem(todoItem) {
 setTodoItems([...getTodoItems(), todoItem]);
 }
 function getTodoItem(id) {
 return getTodoItems().filter(todoItem => todoItem.id === id)[0];
 }
 function updateTodoItem(todoItem) {
 const allTodoItems = getTodoItems();
 const itemIndex = allTodoItems.findIndex(item => item.id === todoItem.id);
 allTodoItems[itemIndex].text = todoItem.text;
 allTodoItems[itemIndex].id = todoItem.id;
 allTodoItems[itemIndex].completed = todoItem.completed;
 setTodoItems(allTodoItems);
 }
 return {
 getNextTodoItemId,
 addTodoItem,
 getTodoItems,
 getTodoItem,
 updateTodoItem,
 deleteTodoItem,
 deleteTodoItems
 }
}());

todoService.js

import todoItemsStorageService from './todoItemsStorageService.js';
export default (function() {
 function Todo(text) {
 this.text = text;
 this.id = `todo-item-${todoItemsStorageService.getNextTodoItemId()}`;
 this.completed = false;
 todoItemsStorageService.addTodoItem(this);
 }
 function toggleTodoItemCompletedStatus(id) {
 const todoItem = todoItemsStorageService.getTodoItem(id);
 todoItem.completed = !todoItem.completed;
 todoItemsStorageService.updateTodoItem(todoItem);
 }
 const getTodoItems = () => todoItemsStorageService.getTodoItems();
 const deleteTodoItem = (id) => todoItemsStorageService.deleteTodoItem(id);
 const deleteTodoItems = () => todoItemsStorageService.deleteTodoItems();
 return {
 Todo,
 getTodoItems,
 toggleTodoItemCompletedStatus,
 deleteTodoItem,
 deleteTodoItems
 };
}());

todoController.js


(function() {
 const findById = id => document.querySelector(`#${id}`);
 const createElement = (tag, options) => document.createElement(tag, options);
 window.addEventListener('load', function() {
 refreshUi();
 });
 findById('new-todo-input').addEventListener('keydown', e => {
 if ('Enter' === e.key && e.target.value) {
 new todoService.Todo(e.target.value);
 e.target.value = '';
 refreshUi();
 }
 });
 findById('delete-all-todo-items').addEventListener('click', e => {
 todoService.deleteTodoItems();
 refreshUi();
 });
 function toggleTodoCompleteStatus(id) {
 todoService.toggleTodoItemCompletedStatus(id);
 refreshUi();
 }
 function deleteTodoItem(id) {
 todoService.deleteTodoItem(id);
 refreshUi();
 }
 function createTodoDiv(todoItem) {
 const todoDiv = createElement('div');
 todoDiv.id = todoItem.id;
 const markAsDoneCheckbox = createElement('input');
 markAsDoneCheckbox.setAttribute('type', 'checkbox');
 markAsDoneCheckbox.addEventListener('click', () => toggleTodoCompleteStatus(todoItem.id));
 const todoSpan = createElement('span');
 todoSpan.innerText = todoItem.text;
 const deleteTodoItemLink = createElement('a');
 deleteTodoItemLink.setAttribute('href', '#');
 deleteTodoItemLink.addEventListener('click', () => deleteTodoItem(todoItem.id));
 deleteTodoItemLink.innerText = '❌';
 if (todoItem.completed) {
 todoSpan.classList.add('done');
 markAsDoneCheckbox.setAttribute('checked', 'checked');
 }
 todoDiv.appendChild(markAsDoneCheckbox);
 todoDiv.appendChild(todoSpan);
 todoDiv.appendChild(deleteTodoItemLink);
 return todoDiv;
 }
 function refreshUi() {
 const todoItemsDiv = findById('todo-items');
 todoItemsDiv.innerHTML = '';
 todoService.getTodoItems().forEach(todoItem => todoItemsDiv.appendChild(createTodoDiv(todoItem)));
 }
})();

Question 2

Poor naming

The thing that stood out to me was your naming. Way to wordy. Names only need meaning within the scope they exist in.

General points

window is the default object. You don't use window.document or window.window (LOL to be consistent) so why use it for window.addEventListener ?
Do not use .setAttribute or .getAttribute for properties that are defined by the DOM.
Naming

Many of the names you have created are just too long. Try to keep them to less than 20 characters, and use no more than 2 words.

Capitalize acronyms. UI is an acronym of User Interface, thus refreshUi should be refreshUI
Use arrow function for anon functions.
Don't create a function just to redirect to a function

Example you have window.addEventListener('load', function() { refreshUi(); }); can be addEventListener("load", refreshUI)
Use HTMLElement.textContent rather than HTMLElement.innerText
You started adding functions to reduce DOM API verbosity but stopped short. Much of your code is just repeated name reference noise.
localStorage can be accessed via dot or bracket notation. You don't need to use setItem or getItem

Example

Rather than rewriting your code I have created an example that removes much of the verbosity by using functions.

It still works about the same way. Each change to any todo item refreshes the store and repaints the display.

Snippet notes

As CR snippets do not do modules the code does not include them.
No access to localStorage so replaces it with Object.

const Tag = (name, props = {}) => Object.assign(document.createElement(name), props);
const listener = (el, eName, call, opts ={}) => (el.addEventListener(eName, call, opts), el);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.append(sib), p), par);
const localStore = (() => { try { return localStorage } catch(e) { return {} } })();
const todoStorage = (() => {
 const todos = new Map();
 function reset() {
 todos.clear();
 save();
 localStore.UId = 1;
 }
 const save = () => localStore.todoItems = JSON.stringify([...todos.values()]);
 function load() {
 if (!localStore.todoItems) { reset() }
 JSON.parse(localStore.todoItems).forEach(todo => todos.set(todo.id, todo));
 }
 load();
 return Object.freeze({
 get UId() { return Number(localStore.UId = Number(localStore.UId) + 1) },
 delete(id) {
 todos.delete(id);
 save();
 },
 add(...items) {
 for(const todo of items) { todos.set(todo.id, {...todo}) }
 save();
 },
 byId(id) { return todos.has(id) ? {... todos.get(id)} : undefined },
 asArray() { return [...todos.values()].map(todo => ({...todo})) },
 reset, 
 });
})();
const todoService = (() => {
 const store = todoStorage;
 return Object.freeze({
 Todo(text, completed = false) {
 const todo = { text, completed, id: store.UId };
 store.add(todo);
 return todo;
 },
 get all() { return store.asArray() },
 toggleComplete(id) {
 const todo = store.byId(id);
 if (todo) {
 todo.completed = !todo.completed;
 store.add(todo);
 }
 },
 delete: todoStorage.delete,
 reset: todoStorage.reset,
 });
})();
(() => {
 const UIEvent = (call, ...args) => e => (call(e, ...args) !== false && refreshUI());
 const actions = {
 enter(e) {
 if (e.key === "Enter") {
 service.Todo(e.target.value);
 e.target.value = '';
 } else { return false }
 },
 toggleComplete(e, id) { service.toggleComplete(id) },
 delete(e, id) { service.delete(id) }
 };
 const service = todoService;
 addEventListener('load', refreshUI); 
 listener(newTodoInput, "keydown", UIEvent(actions.enter));
 listener(deleteAllTodoItems, "click", UIEvent(service.reset)); 
 function createTodo({id, text, completed}) { 
 return append( 
 Tag("div"),
 listener( 
 Tag("input", {type: "checkbox", checked: completed}), 
 "click", UIEvent(actions.toggleComplete, id)
 ),
 Tag("span", {textContent: text, className: completed ? "done" : ""}),
 listener(
 Tag("a", {href: "#", textContent: "❌"}), 
 "click", UIEvent(actions.delete, id)
 )
 );
 }
 function refreshUI() {
 todoItems.innerHTML = "";
 append(todoItems, ...service.all.map(createTodo));
 }
})();

.done {
 color: red;
}

<div>
 <input id="newTodoInput" type="text">
 <h2>To Do Items</h2>
 <div id="todoItems"></div>
 <hr/>
 <a href="#" id="deleteAllTodoItems">Delete All</a>
</div>

Question 3

Thank you for your valuable time and your response. Definitely there are many points that I can apply to my code. If it is ok with you, there are some points I do not agree with. (Once I told something like this and it was not well received by a reviewer.) I am trying to understand the code you written now and will be applying most of your suggestions to my own implementation.

Question 4

I applied some of the suggestions you made: github.com/koraytugay/vanillatodo/pull/1/files in case you are interested. For some of your suggestions, I prefer otherwise. I again thank you for your valuable input.

score 2 · Accepted Answer · 2021-08-30 00:13:56Z

Poor naming

The thing that stood out to me was your naming. Way to wordy. Names only need meaning within the scope they exist in.

General points

window is the default object. You don't use window.document or window.window (LOL to be consistent) so why use it for window.addEventListener ?
Do not use .setAttribute or .getAttribute for properties that are defined by the DOM.
Naming

Many of the names you have created are just too long. Try to keep them to less than 20 characters, and use no more than 2 words.

Capitalize acronyms. UI is an acronym of User Interface, thus refreshUi should be refreshUI
Use arrow function for anon functions.
Don't create a function just to redirect to a function

Example you have window.addEventListener('load', function() { refreshUi(); }); can be addEventListener("load", refreshUI)
Use HTMLElement.textContent rather than HTMLElement.innerText
You started adding functions to reduce DOM API verbosity but stopped short. Much of your code is just repeated name reference noise.
localStorage can be accessed via dot or bracket notation. You don't need to use setItem or getItem

Example

Rather than rewriting your code I have created an example that removes much of the verbosity by using functions.

It still works about the same way. Each change to any todo item refreshes the store and repaints the display.

Snippet notes

As CR snippets do not do modules the code does not include them.
No access to localStorage so replaces it with Object.

const Tag = (name, props = {}) => Object.assign(document.createElement(name), props);
const listener = (el, eName, call, opts ={}) => (el.addEventListener(eName, call, opts), el);
const append = (par, ...sibs) => sibs.reduce((p, sib) => (p.append(sib), p), par);
const localStore = (() => { try { return localStorage } catch(e) { return {} } })();
const todoStorage = (() => {
 const todos = new Map();
 function reset() {
 todos.clear();
 save();
 localStore.UId = 1;
 }
 const save = () => localStore.todoItems = JSON.stringify([...todos.values()]);
 function load() {
 if (!localStore.todoItems) { reset() }
 JSON.parse(localStore.todoItems).forEach(todo => todos.set(todo.id, todo));
 }
 load();
 return Object.freeze({
 get UId() { return Number(localStore.UId = Number(localStore.UId) + 1) },
 delete(id) {
 todos.delete(id);
 save();
 },
 add(...items) {
 for(const todo of items) { todos.set(todo.id, {...todo}) }
 save();
 },
 byId(id) { return todos.has(id) ? {... todos.get(id)} : undefined },
 asArray() { return [...todos.values()].map(todo => ({...todo})) },
 reset, 
 });
})();
const todoService = (() => {
 const store = todoStorage;
 return Object.freeze({
 Todo(text, completed = false) {
 const todo = { text, completed, id: store.UId };
 store.add(todo);
 return todo;
 },
 get all() { return store.asArray() },
 toggleComplete(id) {
 const todo = store.byId(id);
 if (todo) {
 todo.completed = !todo.completed;
 store.add(todo);
 }
 },
 delete: todoStorage.delete,
 reset: todoStorage.reset,
 });
})();
(() => {
 const UIEvent = (call, ...args) => e => (call(e, ...args) !== false && refreshUI());
 const actions = {
 enter(e) {
 if (e.key === "Enter") {
 service.Todo(e.target.value);
 e.target.value = '';
 } else { return false }
 },
 toggleComplete(e, id) { service.toggleComplete(id) },
 delete(e, id) { service.delete(id) }
 };
 const service = todoService;
 addEventListener('load', refreshUI); 
 listener(newTodoInput, "keydown", UIEvent(actions.enter));
 listener(deleteAllTodoItems, "click", UIEvent(service.reset)); 
 function createTodo({id, text, completed}) { 
 return append( 
 Tag("div"),
 listener( 
 Tag("input", {type: "checkbox", checked: completed}), 
 "click", UIEvent(actions.toggleComplete, id)
 ),
 Tag("span", {textContent: text, className: completed ? "done" : ""}),
 listener(
 Tag("a", {href: "#", textContent: "❌"}), 
 "click", UIEvent(actions.delete, id)
 )
 );
 }
 function refreshUI() {
 todoItems.innerHTML = "";
 append(todoItems, ...service.all.map(createTodo));
 }
})();

.done {
 color: red;
}

<div>
 <input id="newTodoInput" type="text">
 <h2>To Do Items</h2>
 <div id="todoItems"></div>
 <hr/>
 <a href="#" id="deleteAllTodoItems">Delete All</a>
</div>

Thank you for your valuable time and your response. Definitely there are many points that I can apply to my code. If it is ok with you, there are some points I do not agree with. (Once I told something like this and it was not well received by a reviewer.) I am trying to understand the code you written now and will be applying most of your suggestions to my own implementation.
I applied some of the suggestions you made: github.com/koraytugay/vanillatodo/pull/1/files in case you are interested. For some of your suggestions, I prefer otherwise. I again thank you for your valuable input.

Stack Exchange Network

Todo App with JavaScript

1 Answer 1

Poor naming

General points

Example

Snippet notes

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

Todo App with JavaScript

1 Answer 1

Poor naming

General points

Example

Snippet notes

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions