72

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

Syscall
  • 19,327
  • 10
  • 37
  • 52
Grahame A
  • 3,903
  • 12
  • 46
  • 70
  • Do your "accumulator" fields start out with empty values? If so, that's where your `NaN` results are coming from. – Pointy Dec 22 '10 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. – user113716 Dec 22 '10 at 17:46
  • .each will do this. A working example http://jsfiddle.net/Lijo/Hb28u/16/ – LCJ Feb 18 '13 at 15:43
  • Thanks for your comment, but this was answered 2 years ago... – Grahame A Feb 19 '13 at 16:09

7 Answers7

152

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.

Syscall
  • 19,327
  • 10
  • 37
  • 52
lonesomeday
  • 233,373
  • 50
  • 316
  • 318
  • 2
    just to add to this you probably want to change your selector to $("#productList li") as well. Rather than using find. – Vadim Dec 22 '10 at 17:11
  • @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. – lonesomeday Dec 22 '10 at 17:13
  • 4
    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. – Vadim Dec 22 '10 at 17:49
  • @Yads Fair enough then -- thanks for the information. I'll edit my answer. – lonesomeday Dec 22 '10 at 17:50
19

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?

girasquid
  • 15,121
  • 2
  • 48
  • 58
  • 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. – exoboy Dec 22 '10 at 17:12
  • It never hurts to be explicit, though - I've been bitten a few too many times by relying on `this`. :) – girasquid Dec 22 '10 at 17:14
  • @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. – Pointy Dec 22 '10 at 17:15
  • 1
    @all: That's why I like this forum... it is nice to see other viewpoints and occasionally learn something new along the way. – exoboy Dec 22 '10 at 17:17
7

Try this:

listItems = $("#productList").find("li").each(function(){
   var product = $(this);
   // rest of code.
});
Conner
  • 30,144
  • 8
  • 52
  • 73
rcravens
  • 8,320
  • 2
  • 33
  • 26
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
}
wsanville
  • 37,158
  • 8
  • 76
  • 101
  • 1
    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. – user113716 Dec 22 '10 at 17:40
  • Ah, good points. I rarely bother to assign `$(this)` to another variable, but did so to keep the OP's code similar. – wsanville Dec 22 '10 at 17:53
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));

    });
exoboy
  • 2,040
  • 2
  • 21
  • 29
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

Philipp
  • 10,240
  • 8
  • 59
  • 71
-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>

ronaldo
  • 1