Simple todo list in Javascript [closed]

Question

I'm looking for someone to tell me how I could refactor/change/improve my JS, I'm still learning and really want my code to be the best it can be for when it comes time for me getting a job in the industry. Would really appreciate a professional's help.

It's just a fairly simple todo list...

Here is the JS

let ul = document.getElementById('list');
let input = document.querySelector("input");
let toggle = document.querySelector('.fa-plus');

toggle.addEventListener('click', function() {
    toggle.classList.toggle('rotated');
    input.classList.toggle('hide');
});

input.addEventListener('keydown', function(e) {
    if (e.keyCode === 13 && input.value !== '') {
        addTodo();
    };
});

let deleted = () => {
    let spans = document.querySelectorAll('span');
    for (let span of spans) {
        span.addEventListener('click', function() {
            this.parentNode.parentNode.removeChild(this.parentNode);
        });
    };
};

let addTodo = () => {
    let listItem = document.createElement('li');
    listItem.innerHTML = '<span><i class="far fa-trash-alt"></i></span> ' + input.value;
    listItem.addEventListener("click", function() {
        this.classList.toggle("lineThrough");
    });
    ul.appendChild(listItem);
    deleted();
    input.value = '';
};

Here is the link to the project on codepen - https://codepen.io/ruaridouglas/pen/mdeaaKV

Answer 1

You declare all your variables with let. You aren't using var, so that's good - but it's better to prefer const over let unless you have no choice but to reassign the variable later. (I recently did a check of one of my scripts and found 414 uses of const compared to 16 uses of let - that's the sort of proportion you want to be aiming towards.) Code is most readable when any reader can immediately understand that a variable is never going to be reassigned from its initial value.

This is a bit opinion-based, but I would prefer to avoid IDs in HTML when possible: they not only automatically create properties on the global object (which can result in strange bugs), but also, only one element with a particular ID can exist in the document at a given time. So, for example, if you were tasked to refactor the code so that multiple lists can be interacted with on a page, you would have to make changes.

(Actually, in new code, I get the feeling that the "industry standard" is to usually avoid vanilla DOM manipulation like this at all - usually, a framework like React is used instead, allowing for the functional composition of components. Eg, if you had a working TodoList, changing to a page with two of them would be as simple as <div><TodoList /></div><div><TodoList /></div>)

You do

<input onblur="this.placeholder = 'What do you want to do?'" onfocus="this.placeholder = ''" placeholder="What do you want to do?" type="text">

Never, ever use inline handlers. They have numerous problems (such as executing inside a few confusing with blocks) and have no place in modern code. To attach an event listener to an element, use addEventListener instead.

But here, you don't need Javascript at all: you can use CSS instead:

input:focus::placeholder {
  color: transparent;
}

You sometimes use functions, and you sometimes use arrow functions. Code is best when its style is consistent - I'd recommend picking one and sticking with it everywhere. (I'd prefer arrow functions, since they're always expressions rather than hoisted and reassignable declarations, and because they're usually more concise, and because they make it clear that this is not being captured in the new function.)

You use KeyboardEvent.keyCode, but it's deprecated. As MDN puts it:

This feature is no longer recommended. Though some browsers might still support it, it may have already been removed from the relevant web standards, may be in the process of being dropped, or may only be kept for compatibility purposes. Avoid using it, and update existing code if possible; see the compatibility table at the bottom of this page to guide your decision. Be aware that this feature may cease to work at any time.

I'd prefer to use the key property instead, which is not only supported, but it's also a whole lot more intuitive when you can simply perform a comparison against a character as a string rather than have to refer to a table mapping keycode numbers to their characters. So, this:

if (e.keyCode === 13 && input.value !== '') {

can be replaced with:

if (e.key === 'Enter' && input.value !== '') {

The deleted function here:

let deleted = () => {
    let spans = document.querySelectorAll('span');
    for (let span of spans) {
        span.addEventListener('click', function() {
            this.parentNode.parentNode.removeChild(this.parentNode);
        });
    };
};

can be improved. First, its name isn't particularly informative - it's good for a function's name to represent what it does. What this function does is it adds an event listener to each <span> in the document, which deletes the span's ancestor when clicked. A better name would be deleteTodosOnTrashClick.

But, it selects all <span>s in the document. So, if you had, say, a header with a <span> in addition to the todo list, if the header's span was clicked on, the header would be removed. It would be better to remove only the <span>s which are children of the todo list:

const spans = ul.querySelectorAll('span');

But you're also adding an event listener to every span, every time a new todo is added. Since these spans are being created dynamically, it would be even better to add an event listener to just the created <span>, at the moment that it's created:

const addTodo = () => {
    const listItem = document.createElement('li');
    listItem.innerHTML = '<span><i class="far fa-trash-alt"></i></span> ' + input.value;
    listItem.addEventListener("click", function() {
        this.classList.toggle("lineThrough");
    });
    ul.appendChild(listItem);
    listItem.querySelector('span').addEventListener('click', () => {
        listItem.remove();
    });
    input.value = '';
};

Note the use of .remove(). This is often preferable to removeChild because removeChild requires a reference to both the parent and the child, which can get a bit verbose, as you can see in your original code.

There's another issue in addTodo. You are creating the HTML string for the new todo by concatenating user input, which should be avoided without taking precautions; this allows the user to execute arbitrary scripts, and insert arbitrary elements, which is a security risk and can result in unpredictable behavior.

For example, imagine if this was hosted on a website for students learning, where the students have logged in, and one of them got a message saying:

Try pasting the following string into the todo list, you won't believe what happens next!

<img onerror="document.body.appendChild(document.createElement('script')).src = 'evil.js'">

(except obfuscated). Then the student could have their login stolen by an attacker, because the site created the HTML by concatenating user input and did not sanitize it beforehand.

Either strip out all HTML tags from the input first, or assign to the textContent of the container for the text instead, or use insertAdjacentText:

listItem.innerHTML = '<span><i class="far fa-trash-alt"></i></span> ';
// ...
listItem.insertAdjacentText('beforeend', input.value);

It would also be a good idea (as always) to run the script in strict mode, and put it inside an IIFE so as to avoid global pollution. All together:

(() => {
  'use strict';
  const ul = document.querySelector('.list');
  const input = document.querySelector("input");
  const toggle = document.querySelector('.fa-plus');
  toggle.addEventListener('click', () => {
    toggle.classList.toggle('rotated');
    input.classList.toggle('hide');
  });

  input.addEventListener('keydown', (e) => {
    if (e.key === 'Enter' && input.value !== '') {
      addTodo();
    };
  });


  const addTodo = () => {
    const listItem = document.createElement('li');
    listItem.innerHTML = '<span><i class="far fa-trash-alt"></i></span> ';
    listItem.addEventListener("click", () => {
      listItem.classList.toggle("lineThrough");
    });
    ul.appendChild(listItem);
    listItem.querySelector('span').addEventListener('click', () => {
      listItem.remove();
    });
    listItem.insertAdjacentText('beforeend', input.value);
    input.value = '';
  };
})();

html {
  -webkit-box-sizing: border-box;
  -moz-box-sizing: border-box;
  box-sizing: border-box;
  font-size: 16px;
}

*,
*:before,
*:after {
  -webkit-box-sizing: inherit;
  -moz-box-sizing: inherit;
  box-sizing: inherit;
}

body,
h1,
h2,
h3,
h4,
h5,
h6,
p,
ol,
ul {
  margin: 0;
  padding: 0;
  font-weight: normal;
}

ol,
ul {
  list-style: none;
}

img {
  max-width: 100%;
  height: auto;
}


/* RESET FINISHED */

@import url('https://fonts.googleapis.com/css2?family=Heebo:wght@500&display=swap');
body {
  font-family: 'Heebo', sans-serif;
  background: rgb(200, 230, 201);
  background: linear-gradient(90deg, #00b09b 0%, #96c93d 100%);
}

#container {
  width: 360px;
  margin: 100px auto;
  background-color: #fff;
  border-radius: 5px;
  box-shadow: 0 0 10px rgba(42, 59, 56, 0.4);
}

h1 {
  font-size: 1.6em;
  font-weight: bold;
  padding: .8em .5em;
  background: #00b09b;
  color: #fff;
}

.fa-plus {
  float: right;
  transition: ease-in .2s;
}

.rotated {
  transform: rotate(45deg);
  -webkit-transform: rotate(45deg);
  -ms-transform: rotate(45deg);
  transition: ease-in .2s;
}

.hide {
  display: none;
}

input {
  clear: left;
  font-size: 1em;
  padding: .8em .5em;
  width: 100%;
  outline: none;
  border-top: none;
  border-left: none;
  border-right: none;
  border-bottom: 1px solid rgba(48, 87, 80, 0.4);
  background-color: rgb(236, 236, 236);
  transition: ease-in .2s;
}

input:focus {
  box-shadow: 0 5px 5px rgba(42, 59, 56, .4);
  border-bottom: none;
}
input:focus::placeholder {
  color: transparent;
}

.lineThrough {
  text-decoration: line-through;
  color: grey;
  transition: ease-in .2s;
}

li {
  width: 100%;
  line-height: 40px;
  transition: ease-in .2s;
}

li:nth-child(2n) {
  background-color: rgb(236, 236, 236);
}

span {
  background-color: #e74c3c;
  height: 40px;
  margin-right: 10px;
  text-align: center;
  color: #fff;
  width: 0;
  display: inline-block;
  transition: 0.2s linear;
  opacity: 0;
}

li:hover span {
  width: 40px;
  opacity: 1;
}

<script src="https://kit.fontawesome.com/391e6a689e.js" crossorigin="anonymous"></script>
<script type="text/javascript" src="assets/js/lib/jquery-3.4.1.min.js"></script>
<div id="container">
  <h1>Javascript To-Do List<i class="fa fa-plus rotated"></i></h1>
  <input placeholder="What do you want to do?" type="text">
  <ul class="list">

  </ul>
</div>