jquery function is not working on loop

Question

I am trying to make a custom validation for my form. We won't know in future how many boltons we are going to add / remove from our form.

It should check if radio buttons are not checked add "error class" to section.

The problem is any radio button I click just effect the last section.

function checkBoltons(){

    var boltons = [1,2,3];

    for (var i of boltons) {
    
  
   var $bolton = $('#bolton-' + i);
   
   if ( $('input[name="data[ObeBolton]['+i+']"]').is(':checked') == true ) {
    
    $bolton.removeClass('section-error');
    
   } else {
    $bolton.addClass('section-error');
    
   }
   
   $('input[name="data[ObeBolton]['+i+']"]:radio').change(function(){
    $('#bolton-' + i).removeClass('section-error');
   });
    }  
}

$(".checkBoltons").click(function(){
    checkBoltons();
})

.section-error {
 background-color: rgb(255, 218, 218);
 border: 1px solid red;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div id="bolton-1">
    <p>bolton-1</p>
    <input type="radio" name="data[ObeBolton][1]" value="1" />
    <input type="radio" name="data[ObeBolton][1]" value="2" /> 
</div>

<div id="bolton-2">
    <p>bolton-2</p>
    <input type="radio" name="data[ObeBolton][2]" value="3" />
    <input type="radio" name="data[ObeBolton][2]" value="4" /> 
</div>

<div id="bolton-3">
    <p>bolton-3</p>
    <input type="radio" name="data[ObeBolton][3]" value="5" />
    <input type="radio" name="data[ObeBolton][3]" value="6" /> 
</div>


<button class="checkBoltons">Check Boltons</button>

The short answer is an adding a scoping function so that `i` is still valid when the events occur much later. The better approach is a re-write to use a single handler (with `data-` attributes as required). — iCollect.it Ltd, Apr 22 '15 at 10:53
http://stackoverflow.com/questions/750486/javascript-closure-inside-loops-simple-practical-example — Arun P Johny, Apr 22 '15 at 10:58
@TrueBlueAussie I have tried your solution but doesn't work with data- parent attribute — Fury, Apr 22 '15 at 10:59

score 2 · Accepted Answer · edited May 23 '17 at 12:05

As @TrueBlueAussie suggested, the solution here is to use a single handler

<div id="bolton-1" class="bolton">
    <p>bolton-1</p>
    <input type="radio" name="data[ObeBolton][1]" value="1" />
    <input type="radio" name="data[ObeBolton][2]" value="2" />
</div>
<div id="bolton-2" class="bolton">
    <p>bolton-2</p>
    <input type="radio" name="data[ObeBolton][2]" value="3" />
    <input type="radio" name="data[ObeBolton][2]" value="4" />
</div>
<div id="bolton-3" class="bolton">
    <p>bolton-3</p>
    <input type="radio" name="data[ObeBolton][3]" value="5" />
    <input type="radio" name="data[ObeBolton][3]" value="6" />
</div>
<button class="checkBoltons">Check Boltons</button>

then

$(".checkBoltons").click(function () {
    $('.bolton').each(function(){
        var checked = $(this).find(':radio').is(':checked');
        $(this).toggleClass('section-error', !checked);        
    })
})
$('.bolton input:radio').change(function () {
    $(this).closest('.bolton').removeClass('section-error');
});

Demo: Fiddle

Now *this* is the way to do it +1 :) – iCollect.it Ltd Apr 22 '15 at 11:06 — iCollect.it Ltd, Apr 22 '15 at 11:06

Shlomi Hassid · Answer 2 · 2015-04-23T09:32:59.513

Quick and easy without changing your HTML:

$(function(){
    //Validate
    $(".checkBoltons").click(function() {
        var elements = $('div[id^="bolton-"]');
        $.each(elements, function(i,ele) {
            if($(ele).find("input:checked" ).length) {
                $(ele).removeClass('section-error');
            } else {
                $(ele).addClass('section-error');
            }
        });
    });
    
    //Remove when fixed
    $('div[id^="bolton-"] input:radio').change(function () {
        $(this).parent().removeClass('section-error');
    });
});

.section-error {
 background-color: rgb(255, 218, 218);
 border: 1px solid red;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.11.1/jquery.min.js"></script>
<div id="bolton-1">
    <p>bolton-1</p>
    <input type="radio" name="data[ObeBolton][1]" value="1" />
    <input type="radio" name="data[ObeBolton][1]" value="2" /> 
</div>

<div id="bolton-2">
    <p>bolton-2</p>
    <input type="radio" name="data[ObeBolton][2]" value="3" />
    <input type="radio" name="data[ObeBolton][2]" value="4" /> 
</div>

<div id="bolton-3">
    <p>bolton-3</p>
    <input type="radio" name="data[ObeBolton][3]" value="5" />
    <input type="radio" name="data[ObeBolton][3]" value="6" /> 
</div>
<button class="checkBoltons">Check Boltons</button>

You missed a couple of shortcuts (using `toggleClass` with a second boolean parameter, and a simple `each()` on `elements` directly), but this is an equally valid improvement to the original mess. +1 — iCollect.it Ltd, Apr 22 '15 at 11:28
@TrueBlueAussie Thank you - I know it can be made allot more clean Exactly as the accepted answer is - I tried to give a more simpler to understand for new developers - also my goal was not changing the HTML structure. — Shlomi Hassid, Apr 22 '15 at 11:37
I applaud your keeping the DOM unchanged. That was appropriate (and was in the answer I threw away). `elements.each()` is actually simpler in this case for newbies. *Side-note: Grammatically you do not prefix "simpler" with "more" as it is already an emphasised version of "simple".* — iCollect.it Ltd, Apr 22 '15 at 11:40

jquery function is not working on loop

2 Answers2