Assignment expression in while condition is a bad practice?

Question

This article explains why I have a warning if I use a code like this:

var htmlCollection = document.getElementsByClassName("class-name"),
    i = htmlCollection.length,
    htmlElement;

// Because htmlCollection is Live, we use a reverse iteration.
while (htmlElement = htmlCollection[--i]) { // **Warning?! Why?!**
    htmlElement.classList.remove("class-name");
}

But this no explaination about « why is a bad practice to assignment expression in a while condition? ».

I also read this stackoverflow answers that point this practice as good. So...

There is a performance problem with while (element = element.parentNode) syntax-like or is just a style-code recommandation?

By the way, seems the « --i » operator is also a bad practice. I read in this article :

The ++ (increment) and -- (decrement) operators have been known to contribute to bad code by encouraging excessive trickiness.

It's some sort of joke?

ShadowRanger · Accepted Answer · 2016-01-06T13:47:28.053

There should be no performance problems with it (arguably, indexing with prefix increment can be slightly slower than postfix increment, due to issues with CPU pipelines; this is a microoptimization so ridiculously micro that it almost certainly means nothing in the context of JS engine overhead, even in C the compiler is likely to reorder expressions if it can to ensure it's not stalled waiting on the increment).

Either way, the main argument against assignment in a conditional is basically that most of the time when you do it, it's a mistake (you meant == or in JS, ===). Some code checkers (and C# requires this as a language feature to avoid accidents) are satisfied if you wrap the assignment in an additional layer of parens, to say, "Yup, I really meant to assign" (which is also necessary when you're comparing the result of the assignment to some other value; omitting the parens would instead compare, then assign a boolean, which even more likely to be wrong).

Some people have a hate on for increment/decrement operators used as part of larger expressions, because remembering order of operations is hard I guess, and because C programmers have been known to write horrible things like ++*++var and the like. I ignore these people; just don't use it for excessively tricky things.

That make sence to point the fact you have « maybe » do an error with `=` instead of `==`. But about assignent performance (not prefix/postfix increment performance), there is no performance problem too? — Bruno J. S. Lesieur, Jan 06 '16 at 13:52
@Haeresis: Any issues it might have would be either negligible, or eliminated by compiler reordering on any reasonably modern JS engine. Only thing that *might* help (profile!) is explicitly testing `while ((htmlElement = htmlCollection[--i]) !== undefined)`, which *might* give the JS JIT-er enough information to optimize the expression down to a simple pointer comparison, but again, this is a microoptimization unlikely to matter in the context of JS overhead. — ShadowRanger, Jan 06 '16 at 14:02

Peter Smith · Answer 2 · 2016-01-06T13:55:35.650

0

As an orthogonal approach, and possible 'cleaner/clearer' there is:

// var htmlCollection = document.getElementsByClassName("class-name");
var htmlCollection = document.querySelectorAll('.class-name');
for(let htmlElement of htmlCollection) {
     htmlElement.classList.remove("class-name");
}

as a method of iterating over DOM elements.

UPDATED to include suggestion from ShadowRanger below.

edited Jan 06 '16 at 13:55

answered Jan 06 '16 at 13:47

Peter Smith

5,528
8
51
77

This method not iterate in reversed way? So I cannot use it in this particular case because if I remove the `class-name`, the item is remove from Live htmlCollection and the next item is not the n+1 but the n+2. Maybe `of` avoid that, but I don't think. – Bruno J. S. Lesieur Jan 06 '16 at 13:50
@Haeresis: It doesn't avoid that. On the other hand, switching to `document.querySelectorAll('.class-name')` would avoid it, [because the `NodeList` returned by that function is a snapshot, not a live view](https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelectorAll). – ShadowRanger Jan 06 '16 at 13:53
Yeah. It's just slower (http://codepen.io/Haeresis/pen/epQLBG and http://codepen.io/Haeresis/pen/epQLve) ;p Thanks! – Bruno J. S. Lesieur Jan 06 '16 at 13:59
And with this ES6 `of` instead `in`, there is no need to use some `hasOwnProperty` check? – Bruno J. S. Lesieur Jan 06 '16 at 14:02
@Haeresis: That test was not apples-to-apples; your `querySelectorAll` was performing additional filtering based on descendants; that costs time, and you're not making it up because you never use the results (where presumably you'd have to manually perform subqueries to get the `.inner div` descendant(s) of each `.vanilla`. `querySelectorAll` would almost certainly win over `getElementsByClassName` if you're following up `getElementsByClassName` with sub-queries for each result. – ShadowRanger Jan 06 '16 at 14:06
Ho yeah, just use `.target` instead `.vanilla .inner div` to see a x2 performance with getElementsByClassName. But if a reverse iteration have x0.5 performance, this is the same thing :) – Bruno J. S. Lesieur Jan 06 '16 at 14:09
@Haeresis: Wholly possible, but again, failing to use the result still skews the test. Static lists are usually cheaper to use, more expensive to create; dynamic lists are cheap to create, but every use has to check for mutations since the last use. For a tiny amount of HTML, that check is likely cheap, so again, your test skews things; real pages tend to be 10s of KB in size minimum, with thousands of tags, not 100 bytes or so with only the tags you're looking for. Bad artificial tests tell you very little about real world performance. – ShadowRanger Jan 06 '16 at 14:14

Assignment expression in while condition is a bad practice?

2 Answers2