2

I know it works with Set, but I was under the impression that it would also work with Array. So I tried it in Chrome and was surprised that it didn't work:

const array = [1,2,3,4,5,6]

for (const item of array) {
    if (item === 3 || item === 4) {
        array.splice(array.indexOf(item), 1);
    }
}

console.log(array) // [1,2,4,5,6]

It didn't delete the 4.

So my question is, is iteration safety only supposed to work with Set and Map, but not Array?

(If this is the case, then other than the simple syntax, I don't see the benefit of using it over for(;;). I was under the impression this for..of was going to prevent bugs, even with Array, like it does with Set and Map)

Note, as a trick, I can do it by cloning the array (or reverse iteration):

const array = [1,2,3,4,5,6]

for (const item of Array.from(array)) {
    if (item === 3 || item === 4) {
        array.splice(array.indexOf(item), 1);
    }
}

console.log(array) // [1,2,5,6]
Kirk Larkin
  • 84,915
  • 16
  • 214
  • 203
trusktr
  • 44,284
  • 53
  • 191
  • 263
  • Have you tried with elements that are not next to each other? This might be a result of 4 moving back to the index of 3 resulting in essentially skip over. The fact that reverse iteration works supports the theory. As for the cloned version, that has no relevance as you are performing two distinct operations on two different arrays that happen to have the same elements. – Avin Kavish Jun 21 '19 at 14:53
  • @AvinKavish Sure, that might work for this particular case, but it works fine with Set (deleting 3 and 4 while iterating). – trusktr Jun 21 '19 at 14:54
  • 1
    Set must have an iterator that is not simply indexed access. – Avin Kavish Jun 21 '19 at 15:00
  • @AvinKavish [Yes indeed](https://stackoverflow.com/a/35943995/1048572) – Bergi Jun 21 '19 at 15:07
  • @Bergi Why was ArrayIterator designed to not be convenient like Set or Map? – trusktr Jun 24 '19 at 05:06
  • @trusktr Because it's not possible. Mutations on an Array work really different from mutations of a Set or Map. `ArrayIterator` instead was designed to be simple and efficient. – Bergi Jun 24 '19 at 06:06
  • @trusktr The best one could come up with was a `remove` method on the iterator instance itself, [like in Java](https://docs.oracle.com/javase/8/docs/api/java/util/Iterator.html#remove--), where the iterator gets to know about the removal of the current element. – Bergi Jun 24 '19 at 06:08

3 Answers3

4

No, (as your example demonstrates) it's not safe to remove elements from an array while iterating it.

The default array iterator stores the current index, and it does not update this index when you call splice on the array. It just continues at the same position, no matter what you did to the elements in the array. You can read the spec for ArrayIterator objects, they basically work like a for (var index=0; index<array.length; index++) yield array[index]; loop.

Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Good to know. Why would `for (const item of [...arr])` work is what keeps puzzling me after reading your reply and the OP question? It uses the same ArrayIterator etc – Akrion Jun 21 '19 at 17:40
  • 1
    @Akrion It uses an `ArrayIterator` as well, but one that iterates a copy of the array. The copy is not affected by modifying `arr`. It's the same as `Array.from(arr)` or `arr.slice()` or `arr.map(x => x)`. – Bergi Jun 21 '19 at 17:46
  • @Akrion Nah, cloning the array just to keep the iteration working is what I'd call a bad practice. One really should simply not remove elements from an array while iterating it, one should create a new array instead. Using `filter`, or iterating the elements to be removed (like in AswinKumar's answer), is much more efficient than `array.splice(array.indexOf(item), 1)` anyway. – Bergi Jun 21 '19 at 17:50
1

That's because when the looping went to 3 (index: 2), the array removes the 3 value, and the 4 now becomes index:2. The next iteration will go to index: 3, which is 5.

You can do it like this instead:

const array = [1,2,3,4,5,6]
for (var i=0;i<array.length;i++) {
    if (array[i] === 3 || array[i] === 4) {
        array.splice(array.indexOf(array[i]), 1);
        --i;
    }
}

console.log(array)
Alvin Theodora
  • 936
  • 1
  • 11
  • 18
1

According to MDN

In general, it is best not to add, modify, or remove properties from the object during iteration, other than the property currently being visited. There is no guarantee whether an added property will be visited, whether a modified property (other than the current one) will be visited before or after it is modified, or whether a deleted property will be visited before it is deleted.

Alternatively, you can try this demo or use filter()

const array = [1, 2, 3, 4, 5, 6]

for (const item of [3, 4]) {
  array.splice(array.indexOf(item), 1);
}

console.log(array)
User863
  • 19,346
  • 2
  • 17
  • 41