CODEWITHSUNDEEP
angularjs
Edward Tanguay
Edward Tanguay

Reputation: 193322

Why is splice not deleting all items inside angular.forEach?

In the following code, when I click on the "Delete All Expired" button, why does it only delete 3 of the 4 customer objects where status=expired?

<html ng-app="mainModule">
    <head>
        <script src="http://code.jquery.com/jquery-1.11.2.min.js"></script>
        <script src="http://ajax.googleapis.com/ajax/libs/angularjs/1.2.26/angular.min.js"></script>
        <script src="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.1/js/bootstrap.min.js"></script>
        <link href="http://maxcdn.bootstrapcdn.com/bootstrap/3.3.1/css/bootstrap.min.css" rel="stylesheet">
    </head>    
    <body ng-controller="mainController" style="padding: 20px 0">

        <div class="col-lg-4">
            <table class="table-striped table-bordered table table-hover">
                <tr>
                    <th>First Name</th>
                    <th>Last Name</th>
                    <th>Status</th>
                </tr>
                <tr ng-repeat="customer in customers| orderBy: 'lastName'">
                    <td>{{customer.firstName}}</td>
                    <td>{{customer.lastName}}</td>
                    <td ng-click="switchStatus(customer)" ng-class="{'text-danger': customer.status == 'expired'}" class="prohibitSelectionClickable">{{customer.status}}</td>
                </tr>
            </table>
            <button ng-click="deleteAllExpired()">Delete all expired</button>
        </div>

        <script>
                    var mainModule = angular.module('mainModule', []);
                            function mainController($scope) {

                                $scope.customers = [
                                    {id: 1, status: 'expired', firstName: 'Joe', lastName: 'Thompson'},
                                    {id: 2, status: 'expired', firstName: 'Hank', lastName: 'Howards'},
                                    {id: 3, status: 'expired', firstName: 'Clara', lastName: 'Okanda'},
                                    {id: 4, status: 'active', firstName: 'Thomas', lastName: 'Dedans'},
                                    {id: 5, status: 'expired', firstName: 'Zoe', lastName: 'Frappe'}
                                ];

                                $scope.deleteAllExpired = function () {
                                    angular.forEach($scope.customers, function (customer, index) {
                                        if (customer.status == 'expired') {
                                            $scope.customers.splice(index, 1);
                                        }
                                    });
                                };

                            }
        </script>
    </body>
</html>

Upvotes: 1

Views: 1792

Answers (2)

Nikos Paraskevopoulos
Nikos Paraskevopoulos

Reputation: 40298

Most probably because splice() mutates the array, and angular.forEach() uses invalid indexes. Better implementation IMO:

$scope.deleteAllExpired = function () {
    var newCustomers = [];
    angular.forEach($scope.customers, function (customer) {
        if (customer.status !== 'expired') {
            newCustomers.push(customer);
        }
    });
    $scope.customers = newCustomers;
};

You may even get a performance gain, not so many mutations (it will probably be negligible for small datasets though).

Upvotes: 2

David Bohunek
David Bohunek

Reputation: 3201

It is because the array you iterate changes (due to deleting) during the iterations and will not go through all the items. Change the method to this:

$scope.deleteAllExpired = function () {

    var i = $scope.customers.length;

    while (i--) {
        var customer = $scope.customers[i];

        if (customer.status == 'expired') {
            $scope.customers.splice(i, 1);
        }
    }   
};

Upvotes: 2

Related Questions