Simple todo list VanillaJS - repository pattern

Question

I'm new in JavaScript, so I was looking for some reviews and advice for better code quality.

import Todo from './Todo.js';

export default class Repository {
  constructor(todos = []) {
    this.todos = todos;
    this.storage = localStorage;
  }

  addTodo(todo, save=true) {
    this.todos = [...this.todos.filter(item => item.uuid !== todo.uuid), todo];
    if (save) {
      this.saveTodos();
    }
  }

  removeTodo(todo) {
    this.todos = [...this.todos.filter(item => item.uuid !== todo.uuid)];
    this.saveTodos();
  }

  saveTodos() {
    this.storage.setItem('todos', JSON.stringify(this.todos));
  }

  loadTodos() {
    const storageTodos = JSON.parse(this.storage.getItem('todos'));
    if (storageTodos !== null) {
      storageTodos.map(todo => this.addTodo(Object.assign(new Todo(), todo)), false);
    }

    return this.todos;
  }
}

https://github.com/adbo/todo_list

Answer 1

General points

• There is no need to add localStorage to the Repository object as it is part of the global this.
• You can access localStorage values directly by the key name ^[.1]. Eg setting localStorage.myData = "Hi World", getting const message = localStorage.myData, to check if it exists localStorage.myData !== undefined, to empty a value localStorage.myData = undefined, and to delete delete localStorage.myData
• LocalStorage should not be trusted. Calling JSON.parse on data from local storage should always be wrapped in a try catch in case the data has been modified by a 3rd party.
• Using filter to remove items from an array is not as efficient as locating the item's index and splicing it. Even better would be to store the todo array as a Map indexed by uuid see second example.
• You have some poor naming due to the repeated prefixing of todo to all the functions. I can imagine that the Repository is called todoList and thus is would be todoList.addTodo todoList.removeTodo and so on. Would be far more readable if you drop the todo . Eg todoList.add, todoList.remove, ...
• You are repeating code. Both add and remove functions filter (remove) a todo. I would be better for add to call remove

Good OO design

Javascripts class syntax is something you should avoid as it forces you to create objects with poor encapsulation (if you don't use the "abomination" that is # private prefix) Javascript has several very robust object patterns that provide air tight encapsulation and should be used if you wish to follow good OO design.

The examples shows and immediately invoked function (IIF) pattern that defines the Object Repository as a function returning an ad hock Object that encapsulates via closure.

The todos array (or map) is isolated and not directly accessible. It uses a getter to return the array of todos as a shallow copy

Examples

Both examples use IIF pattern to encapsulate the object created from the named Repository

Note that it has slightly different behavior than your object.

You can instantiate the object with or without the new token. Eg todos = Repository() or todos = new Repository()

The first example store data in an array that it keeps private, copied on creation and copied when accessed via the getter todos.

IFF ad hock

export default Repository = (() => {
    const save = (todos) => localStorage.todos = JSON.stringify(todos);
    const load = () => {
        try { return localStorage.todos ? JSON.parse(localStorage.todos) : [] }
        catch(e) { return [] }
    }
    return function(todos = []) {
        todos = [...todos]; 
        return Object.freeze({
            get todos() { return [...todos] },
            remove(todo, saveChanges = true) {
                const idx = todos.findIndex(item => item.uuid !== todo.uuid);                
                idx > -1 && todos.splice(idx, 1);  
                saveChanges && save(todos);                
            },                
            add(todo, saveChanges = true) {
                this.remove(todo, false);
                todos.push(toDo);
                saveChanges && save(todos);
            },
            save() { save(todos) },
            load() {
                load().forEach(todo => this.add(Object.assign(new Todo(), todo), false));
                return this.todos;
            }
        });
    }
})();

IFF with Map

This example uses a map to improve the efficiency of removing items and reduces the source size. It behaves identically to the first example.

Also as using this does present a security concern (even in modules) thus example references self via the named constant API

export default Repository = (() => {
    const save = (todos) => localStorage.todos = JSON.stringify(todos);
    const load = () => {
        try { return localStorage.todos ? JSON.parse(localStorage.todos) : [] }
        catch(e) { return [] }
    }
    return function(todosInit = []) {
        const API = Object.freeze({
            get todos() { return [...todos.values()] },
            remove(todo, saveChanges = true) {
                todos.delete(todo.uuid);
                saveChanges && API.save();                
            },                
            add(todo, saveChanges = true) {
                API.remove(todo, false);
                todos.set(todo.uuid, todo);
                saveChanges && API.save();
            },
            save() { save(API.todos) },
            load() {
                load().forEach(todo => API.add(Object.assign(new Todo(), todo), false));
                return API.todos;
            }
        });
        return API;
    }
})();

Notes

^{[.1] Note that MDN localStorage page cites "The pitfalls of using objects as maps" as reason not to use direct key access. Why only warn on localStorage why not have the same warning on every Object? Get with it MDN!!}