patternjavascriptMinor
Simple todo application
Viewed 0 times
todosimpleapplication
Problem
I have been learning HTML/JavaScript and CSS. I have gone through some of the online tutorials, PDFs and books as well. Though, now I am familiar with syntactical stuff, I wasn't sure how I should design or structure code, so I started with a small ToDo application. I didn't want to use any frameworks. As I am still learning, I thought I should spend sometime learning the basics before using any frameworks.
GitHub
```
{
(function (){
//Create a modal, which contains an array to hold list of todos
var modal = {
init: function () {
this.todolist = [];
this.todolist = JSON.parse(localStorage.getItem("tododata"));
if (this.todolist === null)
this.todolist = [];
},
getTodoList: function () {
return this.todolist;
},
setTodoState: function (intodoid) {
//Set status
this.todolist[intodoid].status === "closed" ?
this.todolist[intodoid].status = "open" : this.todolist[intodoid].status = "closed";
localStorage.setItem("tododata", JSON.stringify(this.todolist));
},
setNewTodo: function (inval) {
this.todolist.push({
id: this.todolist.length,
name: inval,
status: "open"
});
localStorage.setItem("tododata", JSON.stringify(this.todolist));
this.myFirebaseRef.push({id: this.todolist.length, name: inval, status: "open"});
},
clearall: function () {
this.todolist = [];
}
}
//Create a View, which interacts with Controller and alters the display in HTML
var view = {
init: function () {
this.todolst = $('#todolistelm');
this.todolst.html(' ');
view.render();
$('#clearall').click(function () {
localStorage.clear();
controller.clearall();
view.render();
GitHub
```
{
(function (){
//Create a modal, which contains an array to hold list of todos
var modal = {
init: function () {
this.todolist = [];
this.todolist = JSON.parse(localStorage.getItem("tododata"));
if (this.todolist === null)
this.todolist = [];
},
getTodoList: function () {
return this.todolist;
},
setTodoState: function (intodoid) {
//Set status
this.todolist[intodoid].status === "closed" ?
this.todolist[intodoid].status = "open" : this.todolist[intodoid].status = "closed";
localStorage.setItem("tododata", JSON.stringify(this.todolist));
},
setNewTodo: function (inval) {
this.todolist.push({
id: this.todolist.length,
name: inval,
status: "open"
});
localStorage.setItem("tododata", JSON.stringify(this.todolist));
this.myFirebaseRef.push({id: this.todolist.length, name: inval, status: "open"});
},
clearall: function () {
this.todolist = [];
}
}
//Create a View, which interacts with Controller and alters the display in HTML
var view = {
init: function () {
this.todolst = $('#todolistelm');
this.todolst.html(' ');
view.render();
$('#clearall').click(function () {
localStorage.clear();
controller.clearall();
view.render();
Solution
From a quick once over:
-
This
could be
essentially you were overwriting
furthermore you should never drop the curly braces after an
-
This:
has a few problems,
- Your first line creates an anonymous function, it offers you the opportunity to pick a great function name that conveys to the reader what he is about to be exposed to like
(function todoApp(){
-
This
init: function () {
this.todolist = [];
this.todolist = JSON.parse(localStorage.getItem("tododata"));
if (this.todolist === null)
this.todolist = [];
},could be
init: function () {
this.todolist = JSON.parse(localStorage.getItem("tododata")) || [];
}essentially you were overwriting
this.todolist = [] withthis.todolist = JSON.parse(localStorage.getItem("tododata"));furthermore you should never drop the curly braces after an
if block- You are copy pasting this line of code all over the place, you should have a dedicated function for this:
localStorage.setItem("tododata", JSON.stringify(this.todolist));-
This:
setTodoState: function (intodoid) {
//Set status
this.todolist[intodoid].status === "closed" ?
this.todolist[intodoid].status = "open" : this.todolist[intodoid].status = "closed";
localStorage.setItem("tododata", JSON.stringify(this.todolist));
},has a few problems,
intodoid is an unfortunate name because it is not lowerCamelCase and because it smells like Hungarian notation. Also the repetition of this.todolist[intodoid].status is too much. I would suggest something like this:toggleTodoState: function (todoId) {
//Toggle status
var status = this.todolist[todoId].status;
this.todolist[todoId].status = status == "closed" ? "open" : "closed";
updateTodoData();
},Code Snippets
init: function () {
this.todolist = [];
this.todolist = JSON.parse(localStorage.getItem("tododata"));
if (this.todolist === null)
this.todolist = [];
},init: function () {
this.todolist = JSON.parse(localStorage.getItem("tododata")) || [];
}setTodoState: function (intodoid) {
//Set status
this.todolist[intodoid].status === "closed" ?
this.todolist[intodoid].status = "open" : this.todolist[intodoid].status = "closed";
localStorage.setItem("tododata", JSON.stringify(this.todolist));
},toggleTodoState: function (todoId) {
//Toggle status
var status = this.todolist[todoId].status;
this.todolist[todoId].status = status == "closed" ? "open" : "closed";
updateTodoData();
},Context
StackExchange Code Review Q#85364, answer score: 3
Revisions (0)
No revisions yet.