Javascript for loop not working properly?

Question

I'm having an issue with a function I've written to "clean" up, see the code below and I'll explain how it works underneath.

clean: function (e) {
    var
            els = null,
        i = 0;

    if (e === undefined) {
        e = this.cont;
    }

    els = e.getElementsByTagName('*');

    for (i=0;i<els.length;i++) {
        if (els[i].className.search('keep') === -1) {
            e.removeChild(els[i]);
        }
    }
    return this;
},

The argument e is a dom element, if it isn't supplied this.cont is also a dom element stored earlier in the whole function and e is defaulted to it.

The function loops through all of the child elements and checks it doesn't have the class keep (fairly obvious what this does) and removes any that don't match.

It all seemed to be working but I have an element which has 2 images and 2 inputs none with the class 'keep' but the variable i only gets to 2 and the loop stops (it should reach 4 and remove all four elements)

any help would be greatly appreciated.

/* UPDATE */

Thanks to @pimvb and and @Brett Walker the final code which works great is below.

clean: function (e) {
    var
        els = null,
        i = 0;

    if (e === undefined) {
        e = this.cont;
    }

    els = e.getElementsByTagName('*');

    i = els.length;

    while (i--) {
        if (els[i].className.search('keep') === -1) {
            els[i].parentNode.removeChild(els[i]);
        }
    }

    return this;
},

put a try / catch around the for loop. What does this report? — Brett Walker
– Brett Walker, Commented Aug 20, 2011 at 12:40
If you're manipulating the HTML dom, you're probably better off using jquery. — Marcin
– Marcin, Commented Aug 20, 2011 at 12:55
I can't use jQuery as I wanted to build this from scratch and not have to worry about library confliction for the people integrating the whole piece of software. All in all jQuery would have convoluted the integration process. I'm working on the answer provided below by @pimvb — Dave Mackintosh
– Dave Mackintosh, Commented Aug 20, 2011 at 13:22

pimvdb · Accepted Answer · 2011-08-20 12:51:48Z

6

The .getElementsByTagName function returns a NodeList which is basically an array but is 'live', which means it's updated automatically if you e.g. remove a child. So when iterating, els.length is changing, resulting in being 2 when you remove 2 children (there are 4 - 2 = 2 left). When having removed 2 children, i == 2 so the loop will end prematurely to what you expect.

To circumvent this and make it a 'static' array, you can convert it into an array like this, which does not update itself:

els = [].slice.call(e.getElementsByTagName('*')); // [].slice.call is a trick to
                                                  // convert something like a NodeList
                                                  // into a static, real array

As Brett Walker pointed out, you can also iterate backwards, like this:

http://jsfiddle.net/pimvdb/cYKxU/1/

var elements = document.getElementsByTagName("a"),
    i = elements.length;

while(i--) { // this will stop as soon as i == 0 because 0 is treated as false
    var elem = elements[i]; // current element

    if(elem.className == "test") // remove if it should be removed
        elem.parentNode.removeChild(elem);
}

This will start at the last element. The .length still gets updated (i.e. becomes less), but this does not matter as you only used it at the beginning, and not during iterating. As a result, you don't suffer from this 'quirk'.

edited Aug 20, 2011 at 12:51

answered Aug 20, 2011 at 12:39

pimvdb

155k80 gold badges313 silver badges358 bronze badges

Sign up to request clarification or add additional context in comments.

4 Comments

Brett Walker Over a year ago

An alternative is to remove from the tail of the node list, moving from the back to the front.

pimvdb Over a year ago

@Brett Walker: That seems interesting, will try that out, thanks.

Dave Mackintosh Over a year ago

Top job guys, the final code works brilliantly. I'd like to vote up but I don't have the privaliges yet. This is the final working code. I couldn't "answer my own question for 7 hours" So had to post it here in the comments. I'll try to remember to re-post clean: function (e) { var els = null, i = 0; if (e === undefined) { e = this.cont; } els = e.getElementsByTagName('*'); i = els.length; while (i--) { if (els[i].className.search('keep') === -1) { els[i].parentNode.removeChild(els[i]); } } return this; }, Thanks again guys, top form.

pimvdb Over a year ago

@Dave Mackintosh: Great you solved it. Perhaps you might want to edit your question with your new code, so that the formatting doesn't get lost.

Collectives™ on Stack Overflow

Javascript for loop not working properly?

1 Answer 1

4 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

4 Comments

Your Answer

Sign up or log in

Post as a guest

Related