HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Vanilla JavaScript ToDo List implementation

Submitted by: @import:stackexchange-codereview··
0
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 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 itself

event.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 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 listContainer

The ` 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 + ">&#10003;</button>";

Context

StackExchange Code Review Q#147064, answer score: 3

Revisions (0)

No revisions yet.