75

I have this block of code

listItems = $("#productList").find("li");

        for (var li in listItems) {
            var product = $(li);
            var productid = product.children(".productId").val();
            var productPrice = product.find(".productPrice").val();
            var productMSRP = product.find(".productMSRP").val();

            totalItemsHidden.val(parseInt(totalItemsHidden.val(), 10) + 1);
            subtotalHidden.val(parseFloat(subtotalHidden.val()) + parseFloat(productMSRP));
            savingsHidden.val(parseFloat(savingsHidden.val()) + parseFloat(productMSRP - productPrice));
            totalHidden.val(parseFloat(totalHidden.val()) + parseFloat(productPrice));

        }

and I'm not getting the desired results - totalItems is coming out as 180+ and the rest all NaN. I suspect its where i use var product = $(li); or perhaps with the expression on the loop itself. Either way - I need to loop through the <li> items in the <ul> labelled #productList

4
  • Do your "accumulator" fields start out with empty values? If so, that's where your NaN results are coming from. Commented Dec 22, 2010 at 17:11
  • The reason you're getting 180+ items, is that a for/in includes prototyped properties, as well as properties of the actual instance. All of the jQuery methods that can be called against a jQuery object are part of the object's prototype. As you now know, .each() is a common way to iterate, though not the only way. Commented Dec 22, 2010 at 17:46
  • .each will do this. A working example jsfiddle.net/Lijo/Hb28u/16 Commented Feb 18, 2013 at 15:43
  • 1
    Thanks for your comment, but this was answered 2 years ago... Commented Feb 19, 2013 at 16:09

7 Answers 7

162

You need to use .each:

var listItems = $("#productList li");
listItems.each(function(idx, li) {
    var product = $(li);

    // and the rest of your code
});

This is the correct way to loop through a jQuery selection.


In modern Javascript you can also use a for .. of loop:

var listItems = $("#productList li");
for (let li of listItems) {
    let product = $(li);
}

Be aware, however, that older browsers will not support this syntax, and you may well be better off with the jQuery syntax above.

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

4 Comments

just to add to this you probably want to change your selector to $("#productList li") as well. Rather than using find.
@Yads Not necessarily. I don't know for certain, but there's every chance that the original code will be more efficient in many browsers, as it doesn't require parsing the string so intensively.
Hmm, I did a few quick benchmarks in jQuery 1.4.2. Using the find method was about 30% slower than the aggregate selector on both IE7 and Chrome.
@Yads Fair enough then -- thanks for the information. I'll edit my answer.
20

You can use each for this:

$('#productList li').each(function(i, li) {
  var $product = $(li);  
  // your code goes here
});

That being said - are you sure you want to be updating the values to be +1 each time? Couldn't you just find the count and then set the values based on that?

4 Comments

There is no need to pass i and li as arguments is there? Maybe I am missing something, but I thought the object being acted upon was passed by default to the function as the this object.
It never hurts to be explicit, though - I've been bitten a few too many times by relying on this. :)
@exoboy you're correct, but it's also true that jQuery passes as arguments the element and the element's index in the selected jQuery list.
@all: That's why I like this forum... it is nice to see other viewpoints and occasionally learn something new along the way.
7

Try this:

listItems = $("#productList").find("li").each(function(){
   var product = $(this);
   // rest of code.
});

Comments

2

Try this code. By using the parent>child selector "#productList li" it should find all li elements. Then, you can iterate through the result object using the each() method which will only alter li elements that have been found.

listItems = $("#productList li").each(function(){

        var product = $(this);
        var productid = product.children(".productId").val();
        var productPrice = product.find(".productPrice").val();
        var productMSRP = product.find(".productMSRP").val();

        totalItemsHidden.val(parseInt(totalItemsHidden.val(), 10) + 1);
        subtotalHidden.val(parseFloat(subtotalHidden.val()) + parseFloat(productMSRP));
        savingsHidden.val(parseFloat(savingsHidden.val()) + parseFloat(productMSRP - productPrice));
        totalHidden.val(parseFloat(totalHidden.val()) + parseFloat(productPrice));

    });

Comments

2

For a more definitive answer, you'll need to post some of your markup. You can simplify your jQuery quite a bit, like the following:

$("#productList li").each(function() {
    var product = $(this);
    var productid = $(".productId", product).val();
    var productPrice = $(".productPrice", product).val();
    var productMSRP = $(".productMSRP", product).val();

    // the rest remains unchanged
}

2 Comments

I wouldn't do $(".productId", $(this)) since it's just a less efficient way of doing product.find(".productId"). Also, you cached $(this) but never used it.
Ah, good points. I rarely bother to assign $(this) to another variable, but did so to keep the OP's code similar.
0

To solve this without jQuery .each() you'd have to fix your code like this:

var listItems = $("#productList").find("li");
var ind, len, product;

for ( ind = 0, len = listItems.length; ind < len; ind++ ) {
    product = $(listItems[ind]);

    // ...
}

Bugs in your original code:

  1. for ... in will also loop through all inherited properties; i.e. you will also get a list of all functions that are defined by jQuery.

  2. The loop variable li is not the list item, but the index to the list item. In that case the index is a normal array index (i.e. an integer)

Basically you are save to use .each() as it is more comfortable, but espacially when you are looping bigger arrays the code in this answer will be much faster.

For other alternatives to .each() you can check out this performance comparison: http://jsperf.com/browser-diet-jquery-each-vs-for-loop

1 Comment

Why just not to reduce for-loop like (and remove len variable at all): for ( ind = 0; ind < listItems.length; ind++ ) {
-1

   <script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.4.0/jquery.min.js"></script>
   <script>
      $(document).ready(function() {
      $("form").submit(function(e){
          e.preventDefault();
       var name = $("#name").val();
       var amount =$("#number").val();
       var gst=(amount)*(0.18);
           gst=Math.round(gst);

       var total=parseInt(amount)+parseInt(gst);
       $(".myTable tbody").append("<tr><td></td><td>"+name+"</td><td>"+amount+"</td><td>"+gst+"</td><td>"+total+"</td></tr>");
       $("#name").val('');
       $("#number").val('');

       $(".myTable").find("tbody").find("tr").each(function(i){
       $(this).closest('tr').find('td:first-child').text(i+1);

       });

       $("#formdata").on('submit', '.myTable', function () {

                   var sum = 0;

          $(".myTable tbody tr").each(function () {
                    var getvalue = $(this).val();
                    if ($.isNumeric(getvalue))
                     {
                        sum += parseFloat(getvalue);
                     }                  
                     });

                     $(".total").text(sum);
        });

    });
   });

   </script>

     <style>
           #formdata{
                      float:left;
                      width:400px;
                    }

     </style>
  </head>


  <body>  

       <form id="formdata">  

                           <span>Product Name</span>
                           <input type="text" id="name">
                            <br>

                           <span>Product Amount</span>
                           <input type="text" id="number">
                           <br>
                           <br>
                           <center><button type="submit" class="adddata">Add</button></center>
       </form>
       <br>
       <br>

      <table class="myTable" border="1px" width="300px">
      <thead><th>s.no</th><th>Name</th><th>Amount</th><th>Gst</th><th>NetTotal</th></thead>

      <tbody></tbody>

      <tfoot>
             <tr>
                 <td></td>
                 <td></td>
                 <td></td>
                 <td class="total"></td>
                 <td class="total"></td>
             </tr>

      </tfoot>
      </table>

   </body>

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.