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
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????
}
});
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... ;)
obj.length >>> 0instead of simplyobj.length? \$\endgroup\$i = obj.length >>> 0equalsi = obj.length, i > 0if yes than shouldnt lack of thei > 0part initiate an endless loop? \$\endgroup\$