jashkenas / coffeescript

Unfancy JavaScript
https://coffeescript.org/
MIT License
16.51k stars 1.99k forks source link

Deleting items in a for..in loop #1515

Closed delfick closed 13 years ago

delfick commented 13 years ago

Hello,

Let's say I have the following code

for [cb, remove], index in @queue
    cb()
    if remove then @queue.splice index, 1

Basically, I'm traversing through a list of pairs of callback and a boolean specifying whether the callback should be removed from the list when it's used (some callbacks stay). So as can be seen, if that boolean is true, it is removed from the queue.

The problem I have is it gets compiled to this

var cb, index, remove, _len, _ref, _ref2;
_ref = this.queue;
for (index = 0, _len = _ref.length; index < _len; index++) {
    _ref2 = _ref[index], cb = _ref2[0], remove = _ref2[1];
    cb();
    if (remove) {
        this.queue.splice(index, 1);
    }
}

Where _len doesn't reflect the new length of the queue (_ref in this case) and so the _ref2 = _ref[ndex] will create an error when it gets past the new length of _ref.

Is this a bug? and can be fixed ? Or should I just do

for info, index in @queue
    if info?
        [cb, remove] = info
        cb()
        if remove then @queue.splice index, 1

Thankyou.

Stephen.

ngn commented 13 years ago

I wouldn't expect to be able to modify a collection while iterating it. Your code could be rewritten this way, for instance:

for x in @queue then x[0]() # invoke callbacks
@queue = for x in @queue when not x[1] then x # remove those marked for removal
delfick commented 13 years ago

true. Though that involves looping twice (highly likely to never be a problem in this particular context, but that's beside the point)

I clearly wasn't thinking before and didn't take into account modifying the array mid loop (python background, not that it's an excuse :p) but I can do what I want with a while loop

index = 0
while index < @queue.length
    [cb, remove] = @queue[index]
    cb()
    if remove
        @queue.splice(index, 1)
    else
        index += 1

:)

erisdev commented 13 years ago

There's always Underscore.js if you find yourself doing stuff like this a lot, but it won't let you modify a list in place; it returns a new list with the changes you've made, as per functional style. I like it a great deal.

delfick commented 13 years ago

True, I do use underscore.js, however I can achieve the same effect as each and map with coffeescript constructs.

Either way if I use the loop to construct a new array, I still have to go through the loop again to remove undefined elements, because afaik, there isn't a way to say "don't include this value in resulting array"

On Saturday, July 16, 2011, erisdiscord reply@reply.github.com wrote:

There's always [Underscore.js][_] if you find yourself doing stuff like this a lot, but it won't let you modify a list in place; it returns a copy as per functional style. I like it a great deal.

_: http://documentcloud.github.com/underscore/

Reply to this email directly or view it on GitHub: https://github.com/jashkenas/coffee-script/issues/1515#issuecomment-1581460

michaelficarra commented 13 years ago

there isn't a way to say "don't include this value in resulting array"

continue:

coffee> arr = for i in [1,2,3,4]\
......>   continue if i&1\
......>   i
[ 2, 4 ]
delfick commented 13 years ago

That makes a lot of sense.

I'm disappointed in myself that I didn't think of that.

Thankyou.

On Sat, Jul 16, 2011 at 11:48 AM, michaelficarra reply@reply.github.com wrote:

there isn't a way to say "don't include this value in resulting array"

continue:


coffee> arr = for i in [1,2,3,4]\
......>   continue if i&1\
......>   i
[ 2, 4 ]

Reply to this email directly or view it on GitHub: https://github.com/jashkenas/coffee-script/issues/1515#issuecomment-1585580

erisdev commented 13 years ago

@delfick Actually Underscore has select and its opposite, reject, for removing elements during an iteration.

@michaelficarra …or we could just do it that way and keep it simple. Wow, that is headdeskingly obvious.

mschulkind commented 13 years ago

I'd say this is still an issue. I have some code that's not quite as easy for working around the problem (but of course I will some way).

I say this is an issue because if you wrote the exact same code in pure javascript, it would, as described here: http://stackoverflow.com/questions/3463048/is-it-safe-to-delete-an-object-property-while-iterating-over-them

jmacdonald commented 12 years ago

The aforementioned stack overflow question relates to deleting attributes from an object, not elements from an array. The fundamental problem with the latter is that the length of the array changes after removing the element, and the index is no longer valid as a reference point for your "progress" within the array.

This issue isn't resolved in JavaScript; it's a problem common to both languages.

mschulkind commented 12 years ago

Ah, scratch that. I've really only used coffeescript so I didn't fully understand how the for..in loops worked in coffeescript.