patternjavascriptMinor
Vanilla JavaScript ToDo List implementation
Viewed 0 times
implementationjavascriptlistvanillatodo
Problem
I just started learning JavaScript and I have created a basic TODO list following MVC concepts to apply what I have learnt so far. This is probably overkill for a simple todo app, but the objective is to improve my JavaScript skills.
I'd highly appreciate it if you can spot flaws, especially bad practices and suggest improvements/alternative approaches and basically anything that can help me improve myself further.
Overview of the design
The
event.js
Implements the observer pattern for a specific event. Allows listeners to be attached and notified via prototype functions
item.js
Object representation of an item in a list. Contains additional information for functionality implementation
model.js
Maintains an array of items. Handles the data and the business logic. Notifies the controller of model modification through the observer pattern
```
//MODEL maintains data and handles business logic
function Model(
I'd highly appreciate it if you can spot flaws, especially bad practices and suggest improvements/alternative approaches and basically anything that can help me improve myself further.
Overview of the design
The
view communicates user interactions to the controller via the observer pattern. The controller notifies the model. The model updates the data representation and notifies the controller of the change via the observer pattern. The controller then asks the view to re-render itselfevent.js
Implements the observer pattern for a specific event. Allows listeners to be attached and notified via prototype functions
//Event Dispatcher. maintains a list of observers for a particular event.
//Used for communication between view-controller and model-controller
function Event(notifier){
this.notifier = notifier;
this.listeners = [];
}
//methods to attach and notify listeners
Event.prototype = {
attach: function(listener){this.listeners.push(listener);
},
notify: function(val1,val2){
for (var i=0; i<this.listeners.length; i++){
(this.listeners[i])(val1,val2);}
}
}item.js
Object representation of an item in a list. Contains additional information for functionality implementation
//object representation of an item
function TodoItem(item){
this.item = item; //item name
this.isActive = true; //state maintains active status of item
this.visible = true; //controls visibility of item in the view
}model.js
Maintains an array of items. Handles the data and the business logic. Notifies the controller of model modification through the observer pattern
```
//MODEL maintains data and handles business logic
function Model(
Solution
Overall the design of the code looks fine. Bearing in mind that this code was posted ~3 years ago and you likely have learned quite a bit since then, I have some comments about both the UI and the code.
UI
After adding an item, the text input value persists and the user must manually clear it. Many users would likely welcome having the value cleared after an item is added to the list.
The buttons labeled
Code
Miscellaneous feedback
The code has a fair amount of comments. The indentation is somewhat inconsistent, as some lines are indented with one tab whereas others have an additional tab (or sometimes two - e.g.
Unused variable
The `
1https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id
UI
After adding an item, the text input value persists and the user must manually clear it. Many users would likely welcome having the value cleared after an item is added to the list.
The buttons labeled
all, active and complete could be grouped with a label for Show: (and thus it would make sense to move the reset button to a different line) or else have Show added to the beginning of each label's text.Code
Miscellaneous feedback
The code has a fair amount of comments. The indentation is somewhat inconsistent, as some lines are indented with one tab whereas others have an additional tab (or sometimes two - e.g.
Events notify function). The ES-6 Classes could be used to simplify the objects.Unused variable
listContainerThe `
element is selected by id in the View function:
var listContainer = document.getElementById("mainList");
but it is never used in that function. The render method selects the elements with that tagName:
var ul = (document.getElementsByTagName("ul"))[0];
Perhaps it would be better to store listContainer on this in the View function and utilize it in the render method.
list items with same id attribute
The code that generates the list items creates buttons foreach item:
li.innerHTML = value + "x" + "✓";
Both buttons will thus have the same id attribute, but those should be unique1. Additionally, for backwards-compatibility (with HTML 4) an id value should start with a letter. Perhaps using data attributes would be a better solution.
Function scope
Variable that in the View function could be eliminated if the event handler function was bound to this via Function.bind()`, or converted to an arrow function so it wouldn't get a separate scope.1https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id
Code Snippets
var listContainer = document.getElementById("mainList");var ul = (document.getElementsByTagName("ul"))[0];li.innerHTML = value + "<button name='remove' id=" + i + ">x</button>" + "<button name='complete' id=" + i + ">✓</button>";Context
StackExchange Code Review Q#147064, answer score: 3
Revisions (0)
No revisions yet.