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!!