Monkeypatched code appearing in for...in loop over Array

Question

I have the following code in my web app's main JavaScript:

    // uniq for arrays
if (!Array.prototype.getUnique) {
    Array.prototype.getUnique = function () {
        var u = {}, a = [];
        for (var i = 0, l = this.length; i < l; ++i) {
            if (u.hasOwnProperty(this[i])) {
                continue;
            }
            a.push(this[i]);
            u[this[i]] = 1;
        }
        return a;
    }
}

It's a simple uniq clone for javascript, monkeypatched into the basic Array class. (Not here to debate monkeypatching, flame elsewhere please...)

getUnique() works as intended, but now whenever I iterate over an Array with a for...in loop, an additional index called getUnique is passed to the iterator body, and when this false n is looked up, getUnique's code is the value. (In other words: for...in is looping over the array as it should, but appending a getUnique as the last iteration)

What is going on here? There's another function monkeypatched in right above this one that works fine (indexOf()) and does not appear in the iterator. Here is some example code that triggers this issue:

for (var r in result) {
    //tags = tags + result[r]["tags"].split(" ").join(", ")
    if (result[r]["tags"]) {
        var newtags = result[r]["tags"].split(" ");
        debug.log(newtags);
        for (var n in newtags) {
            debug.log("n is " + n);
            tags.push(newtags[n]);
        }
    }
}

The debug output from this snippet looks like:

[14:22:26.090] [["camp", "furnitur", "wood"]]
[14:22:26.093] ["n is 0"]
[14:22:26.096] ["n is 1"]
[14:22:26.099] ["n is 2"]
[14:22:26.101] ["n is getUnique"]

What is going on here? Refactoring the monkeypatch and dropping it in a utility class is easy enough, but this is a really strange edge case to me. I have some ideas in my head about what is happening here, but those are just guesses. Can anyone explain?

score 1 · Answer 1 · answered Jul 14 '12 at 12:38

1

This is what hasOwnProperty is designed to solve: it tells you whether the name you've iterated to is on the object itself, or is inherited from the prototype.

Try this:

for (var n in newtags) {
    if (newtags.hasOwnProperty(n)) {
        debug.log("n is " + n);
        tags.push(newtags[n]);
    }
}

answered Jul 14 '12 at 12:38

Ned Batchelder

364,293
75
561
662

Ok... but why is for...in only picking up this second patched function? Just above `getUnique()` is an `indexOf()` function, hooked into the same `Array.prototype`, and it does not show in `n`? – Tom Corelis Jul 14 '12 at 12:45

score 1 · Answer 2 · 2012-07-14T12:44:57.397

1

Don't use for-in for iteration of numeric properties.

Use a for loop:

 for (var i = 0; i < newtags.length; i++) {

The for-in statement is almost never the right tool for this job, because...

It includes all properties, including non-numeric, out of range, and prototyped properties.
It makes no guarantee of the order of enumeration.

While you could do a hasOwnProperty check, there's no real benefit, and has some down-sides...

It slows down your iteration
It doesn't help with the order of enumeration issue
Sometimes you want a prototyped index (rare, but it happens). Using hasOwnProperty makes this impossible.

edited Jul 14 '12 at 12:44

answered Jul 14 '12 at 12:39

Is `for-in` safe for hash keys then? I'm not aware of any other way to cleanly iterate over those... – Tom Corelis Jul 14 '12 at 12:50
@TomtheJunglist: `for-in` is pretty much required for non-index-based enumeration. But that's usually not an issue, unless you add an enumerable property to `Object.prototype`, which is widely considered to be a bad practice. But you'll never get a guaranteed order of enumeration, unless you define it with a separate "Array of properties" structure. – Jul 14 '12 at 12:52
Ok... for the moment I care less about the order and more about making sure that I'm only iterating over the things I want to iterate over. Thanks :-) – Tom Corelis Jul 14 '12 at 13:10
Sure, if you don't care about the order, it doesn't matter. `for-in` is still the wrong tool for index iteration in JavaScript. The `for` statement provides the constraint required in most cases. – Jul 14 '12 at 13:29
@TomtheJunglist: By the way, you can skip the loop entirely if you just do this... `tags.push.apply(tags, new_tags)` – Jul 14 '12 at 13:30

score 1 · Answer 3 · answered Jul 14 '12 at 12:43

You shouldn't iterate over an array using for…in loop, that is intended for enumerating properties of objects. So, for instance, you have no guarantee about the order of the indexes, neither – as you saw – that you're iterate only numeric indexes. If someone else adds a property to the Array's prototype in such way, you will iterate that property as well. See https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...in to have the complete picture.

However, in your case, you can do the follow:

for (var n in newtags) {
    if (newtags.hasOwnProperty(n)) {
        debug.log("n is " + n);
        tags.push(newtags[n]);
    }
}

That also prevent to iterate properties that are added by other scripts to the prototype. Also, you could define your function as not enumerable (in the browser where ES5 is supported):

Object.defineProperty(Array.prototype, "getUnique", {
    value: function () { /*.. your function ..*/}
})

See : https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/defineProperty

score 1 · Accepted Answer · edited May 23 '17 at 11:56

The common misconception is that the for...in operation is, in javascript, made to iterate Arrays. This is wrong. It's usage is to iterate over any enumerable property of an object. In javascript, everything is an object (Arrays included). But you may notice, when you do for...in on an array, you don't get values out like length, slice, etc. This is because those are NOT enumerable properties of the object.

This SO question has some really good information on the usage of for...in: Why is using "for...in" with array iteration a bad idea?

If you want to stick to using for...in, you can do like was suggested above and check for hasOwnProperty inside your for...in

for ( var v in myArray ) {
    if ( myArray.hasOwnProperty(v) ) {
        // Do Something
    } 
}

I, however, suggest using plain old boring loops...

for ( var i = 0; i <= myArray.length - 1; i++ ) {
    // Do Something
}

Without going into crazy detail, this works without hitting your added methods because the "indexes" of the array are not really indexes at all, but are instead properties with a name that matches their corresponding index: i.e 1, 0, 4, etc.

They feel like indexes because in javascript you can't access a property with dot notation if that property is a number (i.e: myArray.0 will not work). So you do myArray[0], and that feels like an array.

I like all the answers here but yours explains some of what is actually happening, under the hood. Thank you :-) — Tom Corelis, Jul 14 '12 at 13:11

Monkeypatched code appearing in for...in loop over Array

4 Answers4