How to remove element from array in forEach loop?

novicePrgrmr picture novicePrgrmr · Jul 17, 2014 · Viewed 171.4k times · Source

I am trying to remove an element in an array in a forEach loop, but am having trouble with the standard solutions I've seen.

This is what I'm currently trying:

review.forEach(function(p){
   if(p === '\u2022 \u2022 \u2022'){
      console.log('YippeeeE!!!!!!!!!!!!!!!!')
      review.splice(p, 1);
   }
});

I know it's getting into the if because I'm seeing YippeeeeeE!!!!!!!!!!!!! in the console.

MY PROBLEM: I know that my for loop and if logic are sound, but my attempt to remove the current element from the array is failing.

UPDATE:

Tried out Xotic750's answer, and the element is still not being removed:

Here is the function in my code:

review.forEach(function (item, index, object) {
    if (item === '\u2022 \u2022 \u2022') {
       console.log('YippeeeE!!!!!!!!!!!!!!!!')
       object.splice(index, 1);
    }
    console.log('[' + item + ']');
});

Here is the output where the array is still not removed:

[Scott McNeil]
[reviewed 4 months ago]
[ Mitsubishi is AMAZING!!!]
YippeeeE!!!!!!!!!!!!!!!!
[• • •]

So obviously it is going into the if statement as directed, but it's also obvious that the [• • •] is still there.

Answer

Xotic750 picture Xotic750 · Jul 17, 2014

It looks like you are trying to do this?

Iterate and mutate an array using Array.prototype.splice

var pre = document.getElementById('out');

function log(result) {
  pre.appendChild(document.createTextNode(result + '\n'));
}

var review = ['a', 'b', 'c', 'b', 'a'];

review.forEach(function(item, index, object) {
  if (item === 'a') {
    object.splice(index, 1);
  }
});

log(review);
<pre id="out"></pre>

Which works fine for simple case where you do not have 2 of the same values as adjacent array items, other wise you have this problem.

var pre = document.getElementById('out');

function log(result) {
  pre.appendChild(document.createTextNode(result + '\n'));
}

var review = ['a', 'a', 'b', 'c', 'b', 'a', 'a'];

review.forEach(function(item, index, object) {
  if (item === 'a') {
    object.splice(index, 1);
  }
});

log(review);
<pre id="out"></pre>

So what can we do about this problem when iterating and mutating an array? Well the usual solution is to work in reverse. Using ES3 while but you could use for sugar if preferred

var pre = document.getElementById('out');

function log(result) {
  pre.appendChild(document.createTextNode(result + '\n'));
}

var review = ['a' ,'a', 'b', 'c', 'b', 'a', 'a'],
  index = review.length - 1;

while (index >= 0) {
  if (review[index] === 'a') {
    review.splice(index, 1);
  }

  index -= 1;
}

log(review);
<pre id="out"></pre>

Ok, but you wanted to use ES5 iteration methods. Well and option would be to use Array.prototype.filter but this does not mutate the original array but creates a new one, so while you can get the correct answer it is not what you appear to have specified.

We could also use ES5 Array.prototype.reduceRight, not for its reducing property by rather its iteration property, i.e. iterate in reverse.

var pre = document.getElementById('out');

function log(result) {
  pre.appendChild(document.createTextNode(result + '\n'));
}

var review = ['a', 'a', 'b', 'c', 'b', 'a', 'a'];

review.reduceRight(function(acc, item, index, object) {
  if (item === 'a') {
    object.splice(index, 1);
  }
}, []);

log(review);
<pre id="out"></pre>

Or we could use ES5 Array.protoype.indexOf like so.

var pre = document.getElementById('out');

function log(result) {
  pre.appendChild(document.createTextNode(result + '\n'));
}

var review = ['a', 'a', 'b', 'c', 'b', 'a', 'a'],
  index = review.indexOf('a');

while (index !== -1) {
  review.splice(index, 1);
  index = review.indexOf('a');
}

log(review);
<pre id="out"></pre>

But you specifically want to use ES5 Array.prototype.forEach, so what can we do? Well we need to use Array.prototype.slice to make a shallow copy of the array and Array.prototype.reverse so we can work in reverse to mutate the original array.

var pre = document.getElementById('out');

function log(result) {
  pre.appendChild(document.createTextNode(result + '\n'));
}

var review = ['a', 'a', 'b', 'c', 'b', 'a', 'a'];

review.slice().reverse().forEach(function(item, index, object) {
  if (item === 'a') {
    review.splice(object.length - 1 - index, 1);
  }
});

log(review);
<pre id="out"></pre>

Finally ES6 offers us some further alternatives, where we do not need to make shallow copies and reverse them. Notably we can use Generators and Iterators. However support is fairly low at present.

var pre = document.getElementById('out');

function log(result) {
  pre.appendChild(document.createTextNode(result + '\n'));
}

function* reverseKeys(arr) {
  var key = arr.length - 1;

  while (key >= 0) {
    yield key;
    key -= 1;
  }
}

var review = ['a', 'a', 'b', 'c', 'b', 'a', 'a'];

for (var index of reverseKeys(review)) {
  if (review[index] === 'a') {
    review.splice(index, 1);
  }
}

log(review);
<pre id="out"></pre>

Something to note in all of the above is that, if you were stripping NaN from the array then comparing with equals is not going to work because in Javascript NaN === NaN is false. But we are going to ignore that in the solutions as it it yet another unspecified edge case.

So there we have it, a more complete answer with solutions that still have edge cases. The very first code example is still correct but as stated, it is not without issues.