How to clear selected elements from a list

Question

I am having trouble to make this function work. Instead of deleting all the elements of the TODO list that I select, it deletes half of them. If i have a list with 1 2 3 4 5 6 7 8, it deletes 1 3 5 7.

clearCompleted: function() {
       var clearCompleted = document.getElementById("clearCompleted");
       clearCompleted.addEventListener("click", function() {
         for (var i = 0; i < todoList.todos.length; i++) {
           todoList.todos[i]
           if (todoList.todos[i].completed === true) {
             todoList.deleteTodo(i);
            }
           }
              view.displayTodos()
         })
     }

   deleteTodo: function (position){
      this.todos.splice(position,1);
    },

Answer 1

let todos = [
  {"id": 0, "completed":  true, "name": "done :)"},
  {"id": 1, "completed":  true, "name": "done-skippedThisIndex :("},
  {"id": 2, "completed": false, "name": "working on it"}, 
  {"id": 3, "completed": false, "name": "incomplete"}, 
  {"id": 4, "completed":  true, "name": "finished"},  
  {"id": 5, "completed":  true, "name": "complete-skippedThisIndex :("},
  {"id": 6, "completed":  true, "name": "complete-neverSeesThisIndex :)"},  
  {"id": 7, "completed":  true, "name": "complete-lengthWasShortened :("}
];
//console.log("BEFORE:(length:", todos.length, ")\n", todos);

todos = todos.filter(function(todo, i){
  console.log(i, "of", todos.length, todo);
  return (todo.completed === false);
});

console.log("AFTER:(length:", todos.length, ")\n", todos);

Note: I only passed in i so a console.log could show that all array items were processed to contrast it with to code in the next example which is equivalent to your code.
In reality, you only need the code as below:

todos = todos.filter(function(todo){
  return (todo.completed === false);
});

With ES6 syntax, this could be shortened even further to:

todos = todos.filter(todo => todo.completed === false);

---

Here is the equivelent of your code. If you examine the console.log output, you can see that as you remove items from the array, i no longer matches the id number. This is because as an item advances in the array, the value pointed to by i changes. The result is that what would have been the next item your array looked at, now occupies the spacial location of current i. Now at the next pass of the loop, i increments and the item that moved forward thus never gets looked at. This causes some items to be "skipped" over.

let todos = [
  {"id": 0, "completed":  true, "name": "done :)"},
  {"id": 1, "completed":  true, "name": "done-skippedThisIndex :("},
  {"id": 2, "completed": false, "name": "working on it"}, 
  {"id": 3, "completed": false, "name": "incomplete"}, 
  {"id": 4, "completed":  true, "name": "finished"},  
  {"id": 5, "completed":  true, "name": "complete-skippedThisIndex :("},
  {"id": 6, "completed":  true, "name": "complete-neverSeesThisIndex :)"},  
  {"id": 7, "completed":  true, "name": "complete-lengthWasShortened :("}
];
//console.log("BEFORE:(length:", todos.length, ")\n", todos);

for (var i = 0; i < todos.length; i++) {
  if (todos[i].completed === true) {
    console.log(i,"of", todos.length, ": REMOVING", todos[i]);
    todos.splice(i, 1);
    console.log("    ", todos.length, "(new  length - next id will be skipped over)\n");
  } else {
    console.log(i,"of", todos.length, ": keeping", todos[i]);
    console.log("    ", todos.length, "(same length)\n");
  }
}

console.log("AFTER:(length:", todos.length, ")\n", todos);

---

To keep the looping construct you wrote, simply alter your code so you do not alter the array as you are looping through it. You can create a new array that you populate with the items you want to keep. Then reassign the new array to the old variable:

let todos = [
  {"id": 0, "completed":  true, "name": "done :)"},
  {"id": 1, "completed":  true, "name": "done-skippedThisIndex :("},
  {"id": 2, "completed": false, "name": "working on it"}, 
  {"id": 3, "completed": false, "name": "incomplete"}, 
  {"id": 4, "completed":  true, "name": "finished"},  
  {"id": 5, "completed":  true, "name": "complete-skippedThisIndex :("},
  {"id": 6, "completed":  true, "name": "complete-neverSeesThisIndex :)"},  
  {"id": 7, "completed":  true, "name": "complete-lengthWasShortened :("}
];
//console.log("BEFORE:(length:", todos.length, ")\n", todos);

newTodos = [];
for (var i = 0; i < todos.length; i++) {
  if (todos[i].completed === false) {
    console.log(i,"of", todos.length, ": keeping", todos[i]);
    newTodos.push(todos[i]);
  }
  else {
    console.log(i,"of", todos.length, ": NOT keeping", todos[i]); 
  }
}
todos = newTodos;
console.log("AFTER:(length:", todos.length, ")\n", todos);

Here you can see that all items are actually looked at, no index is skipped.
The result has exactly the items you want.

Another "hacky", NOT recommended method you could use, is to counteract the effect of changing the index that the next item is at, by decrementing the current index when you splice out the current item. This way, at the start of the loop, you would then increment the i to point to the next item you wanted to look at:

let todos = [
  {"id": 0, "completed":  true, "name": "done :)"},
  {"id": 1, "completed":  true, "name": "done-skippedThisIndex :("},
  {"id": 2, "completed": false, "name": "working on it"}, 
  {"id": 3, "completed": false, "name": "incomplete"}, 
  {"id": 4, "completed":  true, "name": "finished"},  
  {"id": 5, "completed":  true, "name": "complete-skippedThisIndex :("},
  {"id": 6, "completed":  true, "name": "complete-neverSeesThisIndex :)"},  
  {"id": 7, "completed":  true, "name": "complete-lengthWasShortened :("}
];
//console.log("BEFORE:(length:", todos.length, ")\n", todos);

for (var i = 0; i < todos.length; i++) {
  if (todos[i].completed === true) {
    console.log(i,"of", todos.length, ": REMOVING", todos[i]);
    todos.splice(i, 1);
    i--;  // decrement i to reflect that the data moved backwards in the array
    console.log("    ", todos.length, "(new length)\n    ", i, "(new i)  decremented i to reflect that the data moved backwards/(to the left by 1) in the array. \n     Now, all items will still be looked at)\n");
  } else {
    console.log(i,"of", todos.length, ": keeping", todos[i]);
    console.log("    ", todos.length, "(same length)\n");
  }
}

console.log("AFTER:(length:", todos.length, ")\n", todos);

This is accomplished by adding i-- immediately after you execute splice(i, 1) to reflect that the remaining data is now located exactly 1 position to the left of where it previously was. This insures that all the data gets looked at and evaluated. This is not the recommended way to handle this situation, but may come in handy for other tricky situations.

for (var i = 0; i < todos.length; i++) {
  if (todos[i].completed === true) {
    todos.splice(i, 1);
    i--;   // all the data moved left, update the index accordingly
  }
}

Answer 2

So the problem would be: you are changing the todo list array when doing the for loop. I will give you a quick example here:

const arr = [1, 2, 3, 4]
for (var i = 0; i < arr.length; i++) {
  console.log('i', i)
  arr.splice(i, 1)
}

console.log(arr) // [2, 4]

If you run this example, the final result of arr would be [2, 4]:

• doing the first iteration, when i = 0, you deleted the first element of arr which is 1, now arr becomes [2, 3, 4]
• doing the second iteration, when i = 1, you deleted the second element of arr, since now arr are changed, it is no longer [1, 2, 3, 4], its [2, 3, 4], the second element is 3, so arr becomes [2, 4]
• doing the third iteration, when i = 2, now arr.length = i, loop finishes

So how to solve this problem:

I would say keep array immutable, do not use splice(), use slice() instead. Or i recommend use filter/map/reduce those "high order function api". Those api will not change original array, and instead return a new array. Here is an article about using "immutable pattern"

Not sure if this solve your problem. Hope helps.