2
\$\begingroup\$

With native JavaScript, I intend to traverse a collection of elements in the DOM that contain a link and an image (and possibly other elements). The image may or may not be inside the link—in most cases the image will be in the link, but in some cases it may be directly adjacent to the link, or it may be a child of another div or span that is adjacent to its relevant link. However, both the link and the image will always be contained within a container element, of which there may be several.

For example, envision a stock photo gallery arranged in a grid layout. The HTML for each item in the gallery's collection may look like any one of the snippets below (simplified for cleanliness and brevity).

It might be:

<div class="item">
  <a href="...">
    <img src="..." />
  </a>
</div>

Or it may be:

<div class="item">
  <img src="..." />
  <a href="...">Image Title</a>
</div>

Or it may be:

<div class="item">
  <span data-blah="...">
    <a href="...">Image Title</a>
  </span>
  <span data-meh="...">
    <img src="..." />
  </span>
</div>

Note: I can't control the given HTML. Nevertheless, I would like to traverse and manipulate the DOM flexibly enough to handle any instance of HTML structure given above. To do so efficiently, I'd first need to build the collection of container elements before traversing, yes?

So this:

var items = document.querySelectorAll('.item'); 
for(i in items) { ... }                           // more efficient

rather than this:

for(i in document.querySelectorAll('.item')) { ... } // less efficient

as the latter would need to re-query the .item selector again inside the loop, which is inefficient. Right?

However, to manipulate the link and image of each item being looped, I still need to access the link and image of said item. To do that, I could use querySelector() on the .item like so:

var items = document.querySelectorAll('.item');
for(i in items) {
  var link = items[i].querySelector('a'), img = items[i].querySelector('img'); 
  // manipulate link and img elements
}

But is triggering querySelector() twice per loop-instance the most efficient approach?

Is there a more efficient approach to access the link and image of each .item element being looped-through than by calling querySelector() twice on each looped .item element?

Consider that the primary intent of manipulation is to change the link URL to the image source URL and add some attributes to the link and/or image. Seems simple enough. I just want to make sure the loop logic is efficient. Ideally, the logic will ultimately be used in either a bookmarklet or a browser plugin. The collection of .item elements could potentially number in the thousands, so I'm hoping to keep this traversal and manipulation process as efficient as possible.

\$\endgroup\$
  • \$\begingroup\$ Your statement that for (i in document.querySelectorAll('.item')) { ... } is less efficient is erroneous. querySelectorAll would not get called once per iteration of the loop. \$\endgroup\$ – 200_success Jan 19 '15 at 12:23
  • \$\begingroup\$ You should use a regular for loop, not for..in. Also note you forgot to declare i. \$\endgroup\$ – elclanrs Jan 19 '15 at 17:49
  • \$\begingroup\$ @200_success: if querySelectorAll would not get called in the loop, how would I access each .item element when inside the loop, according your argument? \$\endgroup\$ – purefusion Jan 19 '15 at 21:09
  • \$\begingroup\$ @elclanrs All suggestions for improvements to the code belong in answers, not in comments. \$\endgroup\$ – 200_success Jan 19 '15 at 21:13
  • 1
    \$\begingroup\$ You're right. It's the ... part that has to be less efficient if you write for (i in document.querySelectorAll('.item')) { ... }. \$\endgroup\$ – 200_success Jan 19 '15 at 21:21
1
\$\begingroup\$

You should not iterate over an array or array-like object using a foreach loop, you should use a good old-fashioned for loop instead:

var items = document.querySelectorAll('.item'); 
for (var i = 0; i < items.length; ++i) { ... }

Yes, this sequential for loop doesn't look as elegant as a foreach loop, but unfortunately, using a foreach loop for arrays in JavaScript is incorrect.

As for this:

var items = document.querySelectorAll('.item'); 
for (var i = 0; i < items.length; ++i) {
  var item = items[i];
  var link = item.querySelector('a');
  var img = item.querySelector('img'); 
}

Having two .querySelector lookups in each iteration is not really a problem, because the lookups are done on a small object, a tiny subset of the entire document.

I wrote the loop body slightly differently from your original:

  • I put the value of items[i] in a local item variable. This is to avoid duplicating item[i]. It's good to eliminate duplications, because if you ever need to change something, you need to do it in all duplicates, which is potentially error-prone.

  • I declared link and img on two lines. This is slightly more verbose, but a bit clearer.

\$\endgroup\$
  • \$\begingroup\$ I was considering asking if a for loop would be better. But why is it better (other than for the sake of backwards compatibility)? I like the syntax of the for-in better than the for loop. I think for loops are ugly. But this is about efficiency, so if they're more efficient then I suppose it should be used. Also, I didn't think there'd be much performance benefit redeclaring items[i] as item on its own. Is there? What about declaring each var individually? Why not on one line as I did in my example? \$\endgroup\$ – purefusion Jan 19 '15 at 21:06
  • \$\begingroup\$ Hi @purefusion, I updated my answer to address the questions in your comment \$\endgroup\$ – Stop ongoing harm to Monica Jan 19 '15 at 21:23
  • \$\begingroup\$ Thanks, I'm aware that for loops are intended for arrays, but technically speaking, querySelector() and related methods return objects, not arrays (according to typeof). So I figured a for-in/foreach loop would be fine. Or do arrays get returned as objects from typeof too? Didn't think to check that. \$\endgroup\$ – purefusion Jan 19 '15 at 21:27
  • \$\begingroup\$ @purefusion you really need to carefully read that linked post. It explains it very nicely. As for what typeof returns, take a look at typeof([]) or typeof(new Array()) \$\endgroup\$ – Stop ongoing harm to Monica Jan 19 '15 at 21:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.