Javascript To Do Application

Question

I've created a simple JS 'To Do List' application. I'm very new to JS and this is the first JS project I have done. I would greatly appreciate any feedback on how to make the code more efficient, neater and better adhere to best practices.

The 'To Do List' uses local storage to persist data, and does simple checks such as seeing if an task with the same name already exists in the list.

// Set Event Listeners
function eventListiners(){
  document.querySelector('#task-form').addEventListener('submit', getTask);
  document.addEventListener('click', onClick); 
};

//Load Event Listeners
eventListiners();

// Update Task List from localStorage on page load
if(localStorage.getItem('tasks') !== null){
  addTasksFromStorage();
}

// click Event
function onClick(){
  /// Delete Item
  if(event.target.className == 'fa fa-remove'){
    removeTask();
  };

  // Clear Tasks
  if(event.target.className == 'clear-tasks btn black'){
    clearTasks(event);
  };
}


// Get Task Value & Pass to storeTask();
function getTask(e){
  let task = this.querySelector('#task').value;

  storeTask(task);
  e.preventDefault();
};


// Store Task in Local Storage
function storeTask(taskValue){
  let task = taskValue;
  let tasks;
  let displayMessage;

  // Check if item exists in localstorage
  if(localStorage.getItem('tasks') === null){
    tasks = [];
  } else {
    tasks = JSON.parse(localStorage.getItem('tasks'));
  };

  // Used to display status message
  displayMessage = document.querySelector('#form-message');

  // Check if task already exists in the list
  if(tasks.includes(task)){
    // Update Status Message
    displayMessage.innerText = 'Item Already Added';
    displayMessage.className = 'message-warning';
  } else {
    // Update Status Message
    displayMessage.innerText = 'Item Added';
    displayMessage.className = 'message-success';

    // Add Task to Tasks Array
    tasks.push(task);
    
    // Pass Task to addTaskToList();
    addTaskToList(task);
  }

  // Update localStorage
  localStorage.setItem('tasks', JSON.stringify(tasks));
};


// Add new task to list of tasks
function addTaskToList(task){
  let taskList;
  let newTask;
  let newTaskLink;
  
  // Select list element
  taskList = document.querySelector('ul.collection');

  // Create new li
  newTask = document.createElement('li');
  newTask.className = 'collection-item';

  newTask.appendChild(document.createTextNode(task));

  newTaskLink = document.createElement('a');
  newTaskLink.className = 'delete-item secondary-content';
  newTaskLink.innerHTML = '<i class="fa fa-remove"></i>';

  // Add new li to list
  newTask.appendChild(newTaskLink);
  taskList.appendChild(newTask);
};


// Remove Task from localStorage & UI
function removeTask(){
  let task;
  let taskIndex;
  let tasks = [];
  
  // Get task li & remove
  task = event.path[2];
  task.remove();

  // Get tasks from localStorage & remove
  tasks = JSON.parse(localStorage.getItem('tasks'));
  taskIndex = tasks.indexOf(task.innerText);
  tasks.splice(taskIndex, 1);
  
  // Update localStorage
  tasks = JSON.stringify(tasks);
  localStorage.setItem('tasks', tasks);
}


// Remove all tasks from localStorage & UI
function clearTasks(e){
  let taskList;

  // Select list parent
  taskList = document.querySelector('ul.collection');

  // Loop through all tasks & remove
  while(taskList.firstChild){
    taskList.removeChild(taskList.firstChild);
  };

  // Clear localStorage
  localStorage.removeItem('tasks');
  e.preventDefault();
};


// Add tasks from localstorage
function addTasksFromStorage(){
  let tasks;

  tasks = JSON.parse(localStorage.getItem('tasks'));

  tasks.forEach(task => {
    addTaskToList(task)
  });
}

<!DOCTYPE html>
<html lang="en">

<head>
  <meta charset="UTF-8">
  <meta name="viewport" content="width=device-width, initial-scale=1.0">
  <meta http-equiv="X-UA-Compatible" content="ie=edge">
  <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/materialize/0.100.2/css/materialize.min.css">
  <link href="https://maxcdn.bootstrapcdn.com/font-awesome/4.7.0/css/font-awesome.min.css" rel="stylesheet" integrity="sha384-wvfXpqpZZVQGK6TAh5PVlGOfQNHSoD2xbE+QkPxCAFlNEevoEH3Sl0sibVcOQVnN"
    crossorigin="anonymous">
  <title>Task List</title>
</head>

<body>
  <div class="container">
    <div class="row">
      <div class="col s12">
        <div id="main" class="card">
          <div class="card-content">
            <span class="card-title">Task List</span>
            <div class="row">
              <form id="task-form">
                <div class="input-field col s12">
                  <input type="text" name="task" id="task" value="Walk the dog">
                  <label for="task">New Task</label>
                </div>
            </div>
            <div class="row">
              <input type="submit" value="Add Task" class="btn">
            </div>
            </form>
            <div class="row">
              <span id="form-message"></span>
            </div>
          </div>
          <div class="card-action">
            <h5 id="task-title">Tasks</h5>
            <ul class="collection">
            </ul>
            <a class="clear-tasks btn black" href="">Clear Tasks</a>
          </div>
        </div>
      </div>
    </div>
  </div>
  <script src="https://code.jquery.com/jquery-3.2.1.js" integrity="sha256-DZAnKJ/6XZ9si04Hgrsxu/8s717jcIzLy3oi35EouyE=" crossorigin="anonymous"></script>
  <script src="https://cdnjs.cloudflare.com/ajax/libs/materialize/0.100.2/js/materialize.min.js"></script>
  <script src="app.js"></script>
</body>

</html>

Sᴀᴍ Onᴇᴌᴀ · Accepted Answer · 2020-03-31 21:00:55Z

I like that the code uses event delegation to determine what action to take based on the element that was clicked.

It is a good habit to use const when declaring a variable until it is determined that re-assignment is necessary- then use let.

There is a typo in the function name eventListiners...

In eventListiners the element with id task-form is selected using querySelector():

document.querySelector('#task-form').addEventListener('submit', getTask);

It is faster to use document.getElementById()¹ or even just for property document.forms[0] (presuming it is the first form on the page- if not, adjust 0 accordingly). Other functions use querySelector when document.getElementById() could be used instead.

The function removeTask() stores an array in tasks, and at the end of the function that variable is overwritten with a string. It might be confusing to someone trying to read your code to keep track of variable types. If the string value was used more than once it would be wise to use a different variable but because it is only used once it can be eliminated by substituting the assigned value where it is used.

The functions onClick() and removeTask() use window.event instead of accepting event as a formal argument (like getTask() does). It is best to accept the event parameter in the handler function instead of referencing that global property.

You should avoid using this property in new code, and should instead use the Event passed into the event handler function. This property is not universally supported and even when supported introduces potential fragility to your code.

Note: This property can be fragile, in that there may be situations in which the returned Event is not the expected value. In addition, Window.event is not accurate for events dispatched within shadow trees. 2.

The function storeTask() sets tasks based on the localStorage values:

if(localStorage.getItem('tasks') === null){
 tasks = [];
} else {
  tasks = JSON.parse(localStorage.getItem('tasks'));
};

This can be simplified to:

const tasks = JSON.parse(localStorage.getItem('tasks')) || [];

The function clearTasks() loops over the list child nodes and removes them. This could be simplified by setting the innerHTML of the list element to an empty string.

taskList.innerHTML = '';

The function onClick() checks Event.target.className for exact values. It would be simpler to use target.classList.contains() - e.g.

if(event.target.classList.contains('fa-remove')) {
  removeTask();
};

// Clear Tasks
if(event.target.classList.contains('clear-tasks')) {

The forEach at the end of addTasksFromStorage() can be simplified from

tasks.forEach(task => {
    addTaskToList(task)
  });

to just:

tasks.forEach(addTaskToList);

Stack Exchange Network

Javascript To Do Application

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Javascript To Do Application

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions