Jake Cross
Jake Cross

Reputation: 523

NodeJS arr.splice() not removing from array

Using the following code, I'm supposed to check if the arrays (ai, jq, and rz) has users with the same 'interest' and if so, remove that user from all arrays. But it seems that the .splice() method is not be working as I log the arrays and they still contain that user. Any ideas?

Code:

function joinQueue(sid, interests, fn) {
	var exists = false;
	
	interests.forEach(function(interest) {
		interest = interest.toLowerCase();
		getTable(interest.charAt(0), function(table) {
			table.forEach(function(data) {
				if(data.interest == interest && !exists) {
					var aa = 0;
					ai.forEach(function(a) {
						if(a.sid == data.sid) { ai.splice(aa, 1); console.log(ai);}
						aa++;
					});
					var jj = 0;
					jq.forEach(function(j) {
						if(j.sid == data.sid) { jq.splice(jj, 1); console.log(jq); }
						jj++;
					});
					var rr = 0;
					rz.forEach(function(r) {
						if(r.sid == data.sid) { rz.splice(rr, 1); console.log(rz); }
						rr++;
					});
					fn(data);
					exists = true;
				}
			});	
		});
	});
	if(!exists) {
		interests.forEach(function(interest) {
			interest = interest.toLowerCase();
			getTable(interest.charAt(0), function(table) {
				table.push({'sid': sid, 'interest': interest});
			});
		});
		fn();
	}
}

Upvotes: 0

Views: 112

Answers (2)

widged
widged

Reputation: 2779

[Untested]. It is difficult to establish whether it makes any difference without access to testable code (input data and some other variables are missing).

It is best to avoid to add or remove items to an array as you are looping over it. Best is to loop over a clone of the array rather than the array itself. Try with the code below to see if it makes any difference.

function joinQueue(sid, interests, fn) {
    var exists = false;
	
    interests.forEach(function(interest) {
        interest = interest.toLowerCase();
        getTable(interest.charAt(0), function(table) {
            table.forEach(function(data) {
                if(data.interest == interest && !exists) {
                    var sid = data.sid;
                    var sliceItOff = function(arr) {
                        arr.slice(0).forEach(function(d, i) {
                            // [warning]. if more than one item meets the condition (d.sid == sid), 
                            // then you are likely to remove an item at the wrong index. 
                            // A better approach is to build a new array rather than modify the old one
                            if(d.sid == sid) { arr.splice(i, 1); console.log(arr); }
                        });
                    };
                    sliceItOff(ai);
                    sliceItOff(jq);
                    sliceItOff(rz);
                    fn(data);
                    exists = true;
				}
			});	
		});
	});
	if(!exists) {
		interests.forEach(function(interest) {
			interest = interest.toLowerCase();
			getTable(interest.charAt(0), function(table) {
				table.push({'sid': sid, 'interest': interest});
			});
		});
		fn();
	}
}

Upvotes: 0

jfriend00
jfriend00

Reputation: 707436

Here's a bit safer version that uses a backwards for loop traversal to avoid issues when the current item is removed.

function joinQueue(sid, interests, fn) {
    var exists = false;

    interests.forEach(function(interest) {
        interest = interest.toLowerCase();
        getTable(interest.charAt(0), function(table) {
            table.forEach(function(data) {
                if(data.interest == interest && !exists) {
                    var sid = data.sid;
                    var sliceItOff = function(arr) {
                        for (var i = arr.length - 1; i >= 0; i--) {
                            if(arr[i].sid == sid) { 
                                arr.splice(i, 1); 
                            }
                        }
                    };
                    sliceItOff(ai);
                    sliceItOff(jq);
                    sliceItOff(rz);
                    fn(data);
                    exists = true;
                }
            }); 
        });
    });
    if(!exists) {
        interests.forEach(function(interest) {
            interest = interest.toLowerCase();
            getTable(interest.charAt(0), function(table) {
                table.push({'sid': sid, 'interest': interest});
            });
        });
        fn();
    }
}

Upvotes: 1

Related Questions