4
\$\begingroup\$

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 ="<h1>" + taskField.value + "</h1>";
        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 < sortedTasksArray.length; i++) {
                taskList.appendChild(sortedTasksArray[i]);
            }
        }

        // DELETE BUTTON
        var deleteButton = document.createElement("button");
        deleteButton.innerText = "Delete";
        newTask.appendChild(deleteButton);
        deleteButton.addEventListener("click", function() {
            taskList.removeChild(newTask);

            if (!newTask.classList.contains("completed")) {
                taskCounter.innerText = parseInt(taskCounter.innerText,10) - 1;
            }
        });

        // COMPLETE BUTTON
        var completeButton = document.createElement("button");
        completeButton.innerText = "Complete";
        newTask.appendChild(completeButton);
        completeButton.addEventListener("click", function() {
            newTask.classList.toggle("completed");
            if (newTask.classList.contains("completed")) {
                taskCounter.innerText = parseInt(taskCounter.innerText,10) - 1;
            } else {
                taskCounter.innerText = parseInt(taskCounter.innerText,10) + 1;
            }
        });

        // TASKCOUNTER +1, AND INPUTS RESET
        taskCounter.innerText = parseInt(taskCounter.innerText,10) + 1;
        taskField.value = "";
        priorityField.value = "";
    };

    // ADD TASK TO ADD BUTTON
    addTaskBtn.addEventListener("click", function() {
        if (taskField.value.length > 5 && taskField.value.length < 100) {
            return taskCreator();
        }
    });

    // ADD TASK TO REMOVE FINISHED TASKS BUTTON
    removeFinishedTasksBtn.addEventListener("click", function() {
        var completedTasks = document.getElementsByClassName("completed");
        var completedTasksArray = toArray(completedTasks);
        for (var i = 0; i < completedTasksArray.length; i++) {
            completedTasks[0].parentNode.removeChild(completedTasks[0]);
        }
    });
.completed {
    color: red;
}
<head>
    <meta charset="UTF-8">
    <title>CodersLab</title>
    <link rel="stylesheet" href="css/style.css">
    <script src="js/app.js"></script>
</head>
<body>

    <input id="taskInput" placeholder="Place your task here"><br>
    <input id="taskPriority" placeholder="Task priority"><br>
    <button id="addTaskButton">Add task</button>

    <p>
        Tasks to finish: <span id="counter">0</span>
    </p>

    <ul id="taskList">
    </ul>

    <button id="removeFinishedTasksButton">Remove finished tasks</button>

</body>

It would be really helpful if someone more experienced than me could look at my code and let me know if there are any major mistakes or bad practice examples. I'll gladly hear any feedback about anything (formatting, code indentation, code structure, logic behind the code and anything you can think of).

Also, I've been thinking about code testing and TDD lately and if you could give me any hint how could i test this "app" (and if it's necessary to test an app which is so small) i'd appreciate it. I haven't read anything about ES6 yet but any info on what could be written "better" thanks to the use of ES6 features will be helpful as well.

\$\endgroup\$
  • \$\begingroup\$ Why write obj.length >>> 0 instead of simply obj.length? \$\endgroup\$ – janos Dec 6 '16 at 23:18
  • \$\begingroup\$ I have to admit that this is the only part of the code (toArray function) which i've copy-pasted from somewhere else. I guess that i = obj.length >>> 0 equals i = obj.length, i > 0 if yes than shouldnt lack of the i > 0 part initiate an endless loop? \$\endgroup\$ – mkrds Dec 6 '16 at 23:27
3
\$\begingroup\$

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 myElement = document.createElement("span");
// append it to the DOM when all data is processed and ready

You should hold that reference for the life of the associated data. If the data changes you make the change to the stored element.

myElement.textContent = "Update"

...and it is automatically reflected in the DOM

If you have a list of elements and you sort them so that the order has changed just append them to the DOM, they will automatically be removed and put in the correct place. This will make it a lot easier to you the coder and the user as the interface will feel a lot more responsive.


  • Define the constants you use in one place. Dont have them scattered in the code

Example you have

 if (!newTask.classList.contains("completed")) {

The string "completed" is used several time in your code. But then someone thinks that completed is not a good class name and changes it. You then have to find each string and change it. As you may also use it as a variable name you cant just replace you have to go by hand and change them all.

const classNames = {
    completed: "completed",
    ... etc
}

then in the code

if (!newTask.classList.contains(classNames.completed)) {         

Bugs

There are a host of potential bugs and flaws

  • Bad array indexing

Error in your code.

// ADD TASK TO REMOVE FINISHED TASKS BUTTON
removeFinishedTasksBtn.addEventListener("click", function() {
    var completedTasks = document.getElementsByClassName("completed");
    var completedTasksArray = toArray(completedTasks);
    for (var i = 0; i < completedTasksArray.length; i++) {
        //=============================================================
        completedTasks[0].parentNode.removeChild(completedTasks[0]); 
        //             ^                                        ^
        // Should they not be i????
    }
});
  • No vetting.

You are querying the DOM for a variety of elements. The DOM is a separate document and likely to be in many forms. The potential for an element to be missing is high. Your app as a whole will fail if any element is missing. The UI parts of the app are an integral part of the code. The better option is to create the UI components in the code. Use the DOM to provide container elements and add the UI to these elements.


Suggestions

I would normally do a rewrite, but in this case there are too many issues and a rewrite would look nothing like your original.


  • Don't keep reading the tasks from the DOM. Read in the tasks once and maintain it in code. Use functions to reflect the changes to the DOM. Dont rely on the layout of the DOM to store your data. Create a javascript object that represent a task, and then maintain an array of these tasks.

This code is an example

<!-- presuming a list item looks something like -->
<li><span class="text">blah blah</span><span class="priority">10</span><span class="status">Pending</span></li>
// define the constants you use in one place. Dont have them scattered in the code
const classNames = {
    test : "text",
    priority : "priority",
    status : "status"

var tasks;  // global to your app;
var dirty = true;  // global flag that indicates the ahave been changes to the internal task list and the
                   // DOM needs to be updated
var globalID = 0;  // You need to give out some ids so you can find objects
function getID(){
    globalID += 1;
    return globalID -1;
}


// creates a task. Has defaults and does not create if data is missing
// returns undefined for error and a task object if good
function createTask(text,priority = 0,status = "Pending"){
    if(text === undefined || typeof text !== "string" || text.trim() === ""){            
        return undefined; /// cant create 
    }
    return {
        text : text,
        priority : priority,
        status : status,
        id : getID()   // get a new ID for this task
    };
}
// loads the tasks from the DOM
function loadTasks(){
    var t,list, text, priority, status, task
    list = document.getElementsByTagName("li");
    tasks = [];  
    dirty = true; // tasks list has changed
    if(liList !== null){
        for(var i = 0; i < liList.length; i ++){      
            var t = liList[i]; 
            text = t.querySelector("." + classNames.text);
            text = text !== null ? text.textContent;
            // do the same for all the fields.

            task = createTask(text, priority, status);
            if(task === undefined){
                // something is wrong warn user and skip this task
            }else{
                task.listItem = t; // add the list item
                addTask(task);
            }
        }
    }
}

  • Create functions to do all the common requirements for a task.

For example the sort task

function sort(field = "priority"){
    tasks.sort((a, b) => a[field] - b[field]);
    dirty = true;
}

  • You will need functions to add, delete, modify a task.

Example of addTask

function addTask(task){ // task is create with create task
    tasks.push(task); // add it to the list
    if(task.listItem === undefined){ // does it have a listItem element
        task.listItem = createListItemForTask(task); // create a list item (dont add to DOM yet)
    }
    task.listItem.task = task; // attach the task to the list item
    dirty = true;
}        

Note that I create and add the list item to the task. Also note that the dirty flag is set to true whenever I make any changes.


  • You should handle all UI via a single function. Use the event object passed to the function to work out what to do. Make the changes then update the UI. The best way is to add a single click event listener to the listItem element. Work out which task the event is associated to. (each list item will have a property task) Workout what the click needs to do. The elements class will help.

As the click events are the only place (currently) that make changes to the task list. This is where you check the dirty flag. If true then update the DOM, remove unwanted object, and finally set the flag to false dirty = false

All the changes to the DOM should be done in one place.


That should do it

Wow that was a lot. I better stop now. If you have question please do ask. The suggestions are not the only way to do things. If you find that it becomes too complex then it is better to do what you feel comfortable in rather than create something you find difficult to manage. Don't get caught up with Best practice, it is the user that comes first, and as long as they are happy you get to program another day... ;)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.