0
\$\begingroup\$

I have a site where a person can buy something. This site is built on a platform that allows any person to build a site without coding knowledge. One of the features of this platform, is that after the person buys something, I as the owner/operator can generate an invoice. All of the products and their details on the invoice are generated by a variable that is specific to the platform, and that I can not change, and therefore have to wait until after the products are generated to do something with them.

I push a button on the GUI of this easy-to-use platform to generate the invoice, and each product and most of it's details end up inside a <tr> with invoice-item-list as the class. Here is an example of what you see when you inspect the page in Chrome (for example). 3 products:

<tr class=" invoice-item-list" data-sku="COM-MON-EXAMP">
    <!--OTHER LINES OF HTML THAT I DON'T NEED TO QUERY-->
    <td class="ProductDetails">
        Example Product 1 | 1X1 Black
        <div class="ProductAttributes" style="">
            <div class="ConfigurableProductRow">
                <div class="Label">Choose Your Accessory Option Below:</div>
                <div class="Value">Option X</div>
                <div class="Value">Option X Description</div>
            </div>
        </div>
    </td>
</tr>
<tr class=" invoice-item-list" data-sku="COM-MON-EXAMP">
    <!--OTHER LINES OF HTML THAT I DON'T NEED TO QUERY-->
    <td class="ProductDetails">
        Example Product 2 | 47in
        <div class="ProductAttributes" style="">
            <div class="ConfigurableProductRow">
                <div class="Label">Choose Your Accessory Option Below:</div>
                <div class="Value">Option YY</div>
                <div class="Value">Option YY Description</div>
            </div>
        </div>
    </td>
</tr>
<tr class=" invoice-item-list" data-sku="COM-MON-EXAMP">
    <!--OTHER LINES OF HTML THAT I DON'T NEED TO QUERY-->
    <td class="ProductDetails">
        Example Product 3 | 1X1 Black
        <div class="ProductAttributes" style="">
            <div class="ConfigurableProductRow">
                <div class="Label">Choose Your Accessory Option Below:</div>
                <div class="Value">Option ZZZ</div>
                <div class="Value">Option ZZZ Description</div>
            </div>
        </div>
    </td>
</tr>

I want to iterate through each of the three <tr>'s pulling out the SKU and desciption from each, and then print them to the page in a specific place next to the product that is detailed within that (or each) <tr>.

An invoice could have one product, it could have 53 products, hence the for loops below.

MY QUESTION: I have been told that querying the DOM inside of a loop should be avoided - consider a for loop with var i = 0 - I find that I can not use i outside of that for loop, and therefore I get stuck doing some kind of querying within the loop.

I have two examples below. The first does all the querying within the loop. The second is kind of a mix, and I can not find a way to exclude querying altogether from the loop. Is number 2 an acceptable method, or do I need to avoid querying within a loop all together?

Code example 1

var orderLength = $(".invoice-item-list").length;
for (var i=0; i < orderLength; i++) {
    var skuValue = $(".invoice-item-list").eq(i).data('sku');
    var accessorySkuValue = $(".ProductDetails").eq(i).find(".Value").eq(0).text().trim();
    //JS HERE TO DO SOMETHING WITH THESE VARIABLES BEFORE THEY GET RE-DEFINED
    //IN THIS CASE, I WILL PRINT THEM TO THE PAGE NEXT TO THE PRODUCT
}

Code example 2

var orderLength = $(".invoice-item-list").length;
var invoiceItem = $(".invoice-item-list");
var productDetails = $(".ProductDetails");

for (var i=0; i < orderLength; i++) {
    var valSku = invoiceItem.eq(i).data('sku');
    var valAccessorySku = productDetails.eq(i).find(".Value").eq(0).text().trim();
    //JS HERE TO DO SOMETHING WITH THESE VARIABLES BEFORE THEY GET RE-DEFINED
    //IN THIS CASE, I WILL PRINT THEM TO THE PAGE NEXT TO THE PRODUCT
}
\$\endgroup\$
  • \$\begingroup\$ On Code Review, we only review code that works correctly as intended, so your bad example would have to be removed. Furthermore, please provide a clearer picture of what this code accomplishes. (What do you really want to do with the results? console.log() seems like a silly example.) It's hard to advise you when we don't have an example of the DOM that this code is working with. \$\endgroup\$ – 200_success Mar 9 '17 at 17:44
  • \$\begingroup\$ I updated the post, is it clear enough? There is a whole lot more to the code but I am trying to keep it as simple as possible because this is a general question not intended to be specifically related to any one piece of code. \$\endgroup\$ – Tron Mar 9 '17 at 22:49
  • \$\begingroup\$ Much better now, thank you! Unlike Stack Overflow, which prefers simplified examples, we prefer to see your real code excerpts with full context. \$\endgroup\$ – 200_success Mar 9 '17 at 22:59
2
\$\begingroup\$

I have been told that querying the DOM inside of a loop should be avoided

That advice must refer to a specific situation which does not apply here (maybe animation or something?).

There's nothing wrong with looping over DOM queries - as you say, that's what makes sense here, as you have a list of things in the DOM.

However, you can clean things up considerably by using more of jQuery's features. For instance, the each function - no need to do a for loop.

Most of all, don't re-do queries if you don't have to. For instance, you have this:

var orderLength = $(".invoice-item-list").length;
var invoiceItem = $(".invoice-item-list");

which could just be written as

var invoiceItem = $(".invoice-item-list");
var orderLength = invoiceItem.length;

avoiding the need to re-query for the .invoice-item-list elements.

And you don't need to find the .ProductDetails ahead of time, since that element's nested under the item element anyway. Moreover, from the looks of the html you posted, there's no ambiguity about what the first .Value element is, meaning you can just go for that directly and skip the .ProductDetails query entirely.

Anyway, as mentioned, you don't need the separate for-loop if you just use each:

$(".invoice-item-list").each(function () {
  var item = $(this);
  var sku = item.data('sku');
  var accessory = item.find('div.Value:first').text().trim();

  // your stuff here...
});
\$\endgroup\$
  • \$\begingroup\$ I am not quite understanding how your example is supposed work. I reconfigured my code but I could not get it to work, so I simplified it by using your example - when I console.log(); the values, I get undefined for sku, i returns "[object HTMLTableRowElement]" and list returns some sort of object which has so many lines I wouldn't know where to start with it. Does the querying need to be done a different way with this method? \$\endgroup\$ – Tron Mar 13 '17 at 17:26
  • \$\begingroup\$ @Tron Wow - yeah, I messed up. I'm too used to JavaScript's built-in forEach where an element from the array is passed in as the first argument. But jQuery's each sets this to the element, and passes the index as the 1st argument (and you don't even need it). I'll update the code \$\endgroup\$ – Flambino Mar 13 '17 at 17:30
  • \$\begingroup\$ okay reconfigured again and I see how it's working but came across a problem. With this method, first, I don't have a counter variable. it seems like every single query I make has to start with that 'item' keyword to reference which $(".invoice-item-list") is being referred to. Problem is that I have several more variables that I define for each line item which don't start with $(".invoice-item-list"), and if I were to change them to follow this method, they would all have to start with item.find("classXY").restOfQuery as opposed to $(".classXY").eq(i).restOfQuery. Does that make sense? \$\endgroup\$ – Tron Mar 13 '17 at 18:13
  • \$\begingroup\$ Let me know if posting ALL the html for each line, and ALL my JS to handle it would help more. As I said before I was trying to keep it simple but it sounds like this is pretty much a case by case basis. \$\endgroup\$ – Tron Mar 13 '17 at 18:38
  • \$\begingroup\$ @Tron You can't update your answer with more code now, since answers have been posted (well, my answer, singular, has been posted), and that's tied to the current question's content. Updating the question would make any answers meaningless. But you can - and are encouraged to - post a new question instead. And yes, always include all relevant code. The above works for what you posted - it can't work for what I didn't know about. \$\endgroup\$ – Flambino Mar 13 '17 at 18:53

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.