JavaScript splice function inside foreach loop decrements index [duplicate]

Question

$scope.clearCompleted = function()
        {
            angular.forEach($scope.todos, function(todo, i)
            {
                if(todo.done)
                {
                    $scope.todos.splice(i, 1);
                }
            });

            if($scope.todos.length == 0)
            {
                $scope.isEmpty = true;
            }
        }

This is my code to delete the 'done' todos from an array, but when two todos after each other are removed, it only removes the second. I think it's because the splice function resets and the returns the spliced array.

Answer 1

You splice elements from the array, which you iterated, therefore indexes in "todos" reduced. Sorry for my bad english.

var notDonedTodos = [];
angular.forEach($scope.todos, function(todo, i)
{
    if(!todo.done)
    {
       notDonedTodos.push(todo);
    }
});

$scope.todos = notDonedTodos;

Answer 2

This is happening because forEach only knows about the initial state of the array, and therefore calls your method twice, even if the first call removes an item from the array. Just do a simple while loop instead:

var i = $scope.todos.length;
while (i--){
    if ($scope.todos[i].done){
        $scope.todos.splice(i, 1);
    }
}

Answer 3

The alternative i've found myself is to use the array.filter method. It is the most easy way to filter an array based on object keys. If you're working on an IE8 project (poor you) you'll need to add a polyfill for this function since it's fairly new to JavaScript.

Everything you need to know about javascript.

Answer code:

$scope.clearCompleted = function() {
    $scope.todos = $scope.todos.filter(function(item) {
        return !item.done;
    });
}

Answer 4

The issue with the each iteration is that it removes an item from the array causing an iteration to be skipped. jQuery has a nice grep method that returns all elements matching a certain criteria that is determined by a provided anonymous function.

var todos =[{id:1, done:false},{id:2, done:true},{id:3, done:true}];

function removeCompleted(todos){
    return $.grep(todos,function(todo){
        return todo.done == false;
    });
}

todos = removeCompleted(todos);
console.log(todos);

Working Example http://jsfiddle.net/ktCEN/

Documentation

Answer 5

As another alternative, you could just decrement your index each time you do a splice. For instance:

$scope.clearCompleted = function() {
    angular.forEach($scope.todos, function(todo, i) {
        if(todo.done) {
            $scope.todos.splice(i, 1);
            i--;
        };
    });

    if($scope.todos.length == 0) {
        $scope.isEmpty = true;
    };
}

This adjusts your index to maintain its validity each time your array is modified. You can still use angular.forEach, and you don't end up with two copies of your array.