Basic JavaScript ToDo list with task priorities

For the sake of training my vanilla JS skills I've decided to make a simple ToDo list with the following features:

• Tasks can be added with a priority (higher the number - higher on the list)

• Counter tells how many tasks are left to be completed

• The "delete" button deletes the according task whether it's completed or not (and lowers the counter if necessary)

• The "complete" button changes the color of task heading to red and lowers the counter

• "Remove finished tasks" removes the tasks which are marked as completed and lowers the counter accordingly

My "app" is obviously ugly but it's not important here.

CodePen



`document.addEventListener("DOMContentLoaded", function() {

var addTaskBtn = document.getElementById("addTaskButton");
var removeFinishedTasksBtn = document.getElementById("removeFinishedTasksButton");
var taskField = document.getElementById("taskInput");
var priorityField = document.getElementById("taskPriority");
var taskList = document.getElementById("taskList");
var taskCounter = document.getElementById("counter");

var toArray = function(obj) {
var array = [];
for (var i = obj.length >>> 0; i--;) {
array[i] = obj[i];
}
return array;
};

var taskCreator = function () {
var existingTasks = document.getElementsByTagName("li");
var existingTasksArray = toArray(existingTasks);
var sortedTasksArray = [];

var newTask = document.createElement("li");
newTask.innerHTML ="" + taskField.value + "";
newTask.dataset.priority = priorityField.value;
existingTasksArray.push(newTask);

// TASKS SORTING
sortedTasksArray = existingTasksArray.sort(function(a, b) {
return b.dataset.priority - a.dataset.priority;
});

// TASKS APPENDING
if (existingTasks.length === 0) {
taskList.appendChild(newTask);
} else {
for (var i = 0; i 5

Not bad but needs improvement.

First I will say 9 out of 10 for style. Not that I am a particulate fan of this style but you have the number one rule of coding under your belt. Consistent style, it is the most important skill any programmer MUST master or you will forever be in the hell of a thousand typo bugs.

I am a fan of the singleton style of design. This encapsulates the toDoList within a single scope. Your code is very similar as you use the DOMContentLoaded event to start the process and have everything inside that function. To make it a little more portable create it as a separate object that can be added to a page as needed and not rely on the DOMContentLoaded event.

Issues.

• Array.sort sorts the array in place and does not create a new array.

You have

var sortedTasksArray = [];

// ... BM67 some other code removed

// TASKS SORTING
sortedTasksArray = existingTasksArray.sort(function(a, b) {
    return b.dataset.priority - a.dataset.priority;
});

You create an array with sortedTasksArray = [] then overwrite it by assigning it the reference to existingTasksArray You should just declare sortedTasksArray and not assign it an array that is not used and immediately dereferenced.

Also I am unsure what your intention is here. Do you expect there to be a second copy of existingTasksArray that is sorted?

-
You have too much going on in the function taskCreator with following list of comments

// TASKS SORTING
// TASKS APPENDING
// DELETE BUTTON
// COMPLETE BUTTON
// TASKCOUNTER +1, AND INPUTS RESET

These are best as separate functions

• Variable names are too verbose. This can make it harder to read the code. A rule of thumb that I tend to use is that if I find I am naming a series of variables with the same prefix that means that I actually have a group of related properties. Rather than have all the independent variable create an object (Javascript objects are so easy to create) put the variables in the object as properties.

For example you have

var existingTasks = document.getElementsByTagName("li");
    var existingTasksArray = toArray(existingTasks);

...that are obviously related. You would be better of with

var existing = {
        tasks : document.getElementsByTagName("li"),
        tasksArray : null,
    }
    existing.tasksArray = toArray(existing.tasks);

But that is an example as I think you should re-factor the whole project.

• Don't declare variables that are only used once and then hold them for the life of the app. This is a needless waste of resources and a potential source of name conflict and bugs. ES6 has let and const that are block scoped that will shorten the life of intermediate variables, or better yet wrap them up in functions.

For example

var addTaskBtn = document.getElementById("addTaskButton");
var removeFinishedTasksBtn = document.getElementById("removeFinishedTasksButton");

...variables are only used once and the names are a little ambiguous addTaskBtn if I saw that I would have to find the definition code to see what it is. Is it a function to add a button, or is it the button/element reference?

• Odd bit of coding with the function toArray. But a classic case of too much cleverness and no smarts.

The function

var toArray = function(obj) {
     var array = [];
     for (var i = obj.length >>> 0; i--;) {
        array[i] = obj[i];
     }
     return array;
 };

I have read the comment that this is a bit of copied code, which is far enough, why rewrite what is already done. You just happened to pick a rather odd code snippet.

The author knew enough to be fancy but not enough to be smart. The obj.length >>> 0 is a short cut method for ensuring a value is an integer though obj.length | 0 would do the same. The i-- in the second for statement means he did not have to add i >= 0 ; i--. The i is evaluated to true or false and if true then 1 is subtracted and the loop statements run.

In reality it should be a while loop. But the worst part is that it is adding elements to an array from top down. This can result in the creation of a sparse array. Sparse arrays are significantly less efficient than a normal arrays. ALWAYS add to an array from the bottom up.

Note that in javascript we have array like objects and this function is to convert from array like to array.

If copying from an array

var array = arrayFrom.map(function(a){return a;});
// or in ES6
var array = arrayFrom.map(a=>a);
// or more ES6
var array = [...arrayFrom];

If copying from an array like object

var array = [];
var i = 0;
while(i < arrayLike.length){
    array[i] = arrayLike[i++]; // dont forget the ++
}

// or in ES6
var array = [...arrayLike];

• DOM elements should be stored with tasks.

Dont recreate DOM elements every time there is a change in the task list. When a new task is created create the listItem as well and store it with the task.

When you create a DOM element

```
var myE

StackExchange Code Review Q#149159, answer score: 3