jQuery iterate or two-step?

Question

I have an array of values of the form:

configs = ["26W", "27W", "28W"...]

They correspond to some of the a elements in a list:

<ul id="value-list"><li><a data-value="26W">...</a></li>
<li><a data-value="27W">...</a></li>...</ul>

When the data in the list does correspond to an existing a, I need to set the parent li class to "available" and not "unavailable". If there is an a with a value not in the list, I need to set the class to "unavailable" and not "available". (Edit: I realize that having both classnames is redundant, but I don't have the option to change that right now.)

I can think of two ways to do this:

Iterate

var $allSwatchItems = $("#value-list > li");
$allSwatchItems.each(function () {
  var $this = $(this);
  var $a = $this.children("a");
  if (configs.indexOf($a.attr("data-value")) === -1) {
    $this.addClass("unavailable").removeClass("available");
  } else {
    $this.addClass("available").removeClass("unavailable");
  }
});

Two-step

var $allSwatchItems = $("#value-list > li");
// Set everything to unavailable at first.
$allSwatchItems.removeClass("available").addClass("unavailable");
// Build the selector for the swatch anchors.
var swatchSelector = configs.map(function (val, idx, arr) {
  return "a[data-value='" + val + "']";
}).join(", ");
// Set the matching list items to available.
var $availSwatchItems = $allSwatchItems.children(swatchSelector).parent();
$availSwatchItems.removeClass("unavailable").addClass("available");

I know there are pros and cons to both approaches, but is one significantly better than the other (and how), or is there a third approach I should consider?

Update: Here's how I finally did it:

function checkAvailable() {
    if (-1 !== $.inArray($(this).children("a").attr("data-value"), configs)) {
        return "available";
    } else {
        return "unavailable";
    };
}
$("ul#value-list > li").removeClass("unavailable available").addClass(checkAvailable);

What's configs? Do you mind about cross browser compatiblity? — Florian Margaine
– Florian Margaine, Commented Aug 8, 2012 at 21:38
configs is the name of the array I mentioned at the very beginning. I have polyfilled Array.prototype.map for old browsers. What other cross-browser issues are there? — kojiro
– kojiro, Commented Aug 8, 2012 at 21:57
indexOf works on arrays starting IE9 only :) developer.mozilla.org/en-US/docs/JavaScript/Reference/… — Florian Margaine
– Florian Margaine, Commented Aug 10, 2012 at 18:33

Borgar · Accepted Answer · 2012-08-09 23:59:17Z

3

I prefer the idea of the second approach, but I think the code in the first is less complicated. The second approach is too hard to understand it at a glance. Therefore the first is significantly better. Clarity is a prerequisite for maintainability.

You don't specify but it looks like there is only a single A per each LI. I'm going on that assumption.

What the selector build does can be written in a more traditional manner:

$("#value-list > li > a[data-value]")
  // assume unavailability
  .parent()
    .removeClass("available")
    .addClass("unavailable")
    .end()
  .filter(function () {
    // is item available?
    return $.inArray( $.attr( this, 'data-value' ), configs ) !== -1;
  })
  // whatever remains is available
  .parent()
    .removeClass("unavailable")
    .addClass("available")
    ;

But I think this is even more elegant and succinct:

$("#value-list > li > a[data-value]")
  // remove all availability classes
  .parent()
    .removeClass("unavailable available")
    .end()
  // add availability classes
  .each(function () {
    // is item available? (avoiding query construction)
    var is_available = $.inArray( $.attr( this, "data-value" ), configs ) !== -1;
    // add appropriate class depending on availability
    $( this.parentNode ).addClass( is_available ? "available" : "unavailable" );
  });

I usually try to keep DOM alterations and query construction to a minimum as these are the most costly things to do. Also Array.indexOf is not available on older MSIE so use $.inArray if you need support for those.

edited Aug 9, 2012 at 23:59

answered Aug 8, 2012 at 22:01

Borgar

1462 bronze badges

\$\begingroup\$ +1 because I really like your last approach, but the .removeClass is removing the classname from the wrong elements. \$\endgroup\$

kojiro
– kojiro

2012-08-08 22:28:51 +00:00
Commented Aug 8, 2012 at 22:28
\$\begingroup\$ Did you test it? There may be something I am missing from the description? I am assuming you only need one or the other of these classes per li and the loop should be reassigning the correct one. \$\endgroup\$

Borgar
– Borgar

2012-08-08 22:38:07 +00:00
Commented Aug 8, 2012 at 22:38
\$\begingroup\$ Yes, but the .removeClass applies to the a, not the li. I'll post an edit to the question showing how I finally did it. It was heavily influenced by your answer. \$\endgroup\$

kojiro
– kojiro

2012-08-09 15:29:20 +00:00
Commented Aug 9, 2012 at 15:29
\$\begingroup\$ *Facepalm!* I knew I should have set up a test case. I have fixed the logic so that it works correctly now (or I would not be able to sleep peacefully). \$\endgroup\$

Borgar
– Borgar

2012-08-10 00:07:02 +00:00
Commented Aug 10, 2012 at 0:07

Add a comment |

Erik Reppen · Accepted Answer · 2012-08-08 23:14:33Z

A good general performance rule of thumb is to avoid hitting the DOM (the HTML-manipulating stuff) more than necessary. Most DOM properties aren't just static properties that get set from somewhere else as things change. Invoking them tends to involve buildup and tear-down and examining of internal state, etc. Anything that actually alters HTML is 99.99% likely to cause reflow calculations.

Why have two classes? Just let the lack of an 'available' when checked for with .hasClass('available') represent 'unavailable'. That way you only hit the DOM when you add 'available' and do the attribute checks with both of those className changes setting off reflow calculations.

A note on the second approach. It looks more concise but it's doing a lot of looping and DOM access under the hood. It's important to understand what JQuery does when invoking it. In this line for instance,

$allSwatchItems.removeClass("available").addClass("unavailable");

JQ is going to have to run down the collection of DOM elements with an internal loop access the className property, do a replace operation on it, set the className property, then access the className property again match for 'unavailable' and add it on if it isn't there.

People tend to shy away from using the class attribute as a place to tie things to data but I'm of the opinion that you're always binding to something in the HTML. Why not use the most efficient thing that's easiest to share with other concerns? And no, I do not consider HTML an exclusive property of the view in MVC and similar patterns. It also represents content and document structure. Leave HTML out of your JavaScript modeling conventions. It has too much overlap with other concerns and other domains. Think of DOM API stuff as low-level and view modeling as higher-level stuff that buries the DOM stuff.

Here's how I'd do it - I'll let you sort out what's worth haggling over with your team/tools where control over HTML/CSS is concerned but by baking things into your HTML structure the right way, it can save a lot of work.

HTML like this with unavailable as default sharing with class with data_val classes if needed (classes can have multiple properties separated by spaces):

<ul id="value-list">
<li class="data_val_26W"><a >...</a></li>
<li class="data_val_27W"><a>...</a></li>...
</ul>

build your available selector:

var swatchSelector = '.data_val_' + config.join(', .data_val_'),
//#value-list step to reduce brutal IE8 DOM tomfoolery
$availableSwatchItems = $('#value-list').find(swatchSelector);

$availableSwatchItems.addClass('available'); //.remove('unavailable') if you must

It's not a minor thing to not have control over your HTML and CSS. By structuring to your advantage, you can drastically reduce code and improve performance. Any tool or back-end dev who makes this impossible is like a stick in the eye for a client-side dev. That's your domain. You should have control over it. As for custom attributes, when browsers have access methods as fast as className and ID for other attributes I don't really care what people use for attributes in HTML5 but for now, it's a pointless performance-hamperer, IMO.

Heh, I should've mentioned I don't get to pick the classnames. Anyway, +1. — kojiro
– kojiro, Commented Aug 8, 2012 at 21:56
By the way, could you be more explicit about not hitting the DOM more often than necessary? I would think my two-step approach hits the DOM less often, but what do you think? — kojiro
– kojiro, Commented Aug 8, 2012 at 22:00
At a glance I'd say you're hitting the DOM properties more in the second one. Also, attribute selectors 'a[data-value=something]' are much slower than ID, tag, and class selectors. (although class selectors stink in IE<9) — Erik Reppen
– Erik Reppen, Commented Aug 8, 2012 at 22:10
I'll add some to think in terms of what JQ is actually doing. — Erik Reppen
– Erik Reppen, Commented Aug 8, 2012 at 22:11

delete me · Accepted Answer · 2012-08-10 03:08:30Z

The solution you edited into your question can be improved upon (shortened and sped up) by using an object for configs instead of an array. After all, you're just using it as a lookup now - and an array is one of the worst ways to store a simple lookup.

Here's a complete implementation.

var configs = {'26W':1, '27W':1, '28W':1, ...};
$("ul#value-list > li").removeClass("unavailable available").addClass(function () {
    return configs[$(this).children('a').attr('data-value')] ? 'available' : 'unavailable';
});

Alternatively, use configs.hasOwnProperty(x) ? y : z; if you're concerned about Object.prototype being modified.

Stack Exchange Network

jQuery iterate or two-step?

Iterate

Two-step

3 Answers 3

You must log in to answer this question.

Hot Network Questions

jQuery iterate or two-step?

Iterate

Two-step

3 Answers 3

You must log in to answer this question.

Related

Hot Network Questions