current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

I have a table with 'nested' inputs that need to be checked or unchecked when a button 'select all' is pressed. There are several levels that can be toggled with their respective 'select all' button. In my code I have a repetitive .each() statement where the second one tests if there are differing numbers of checked/unchecked inputs and if there are it overwrites what the first .each() statement did. How can I write the following code better?

$(document).ready(function(){
    $(".checkAll").click(function(e){
        e.preventDefault();
        var thisRow = $(this).parents("tr");
        var thisRowNumber = parseInt(thisRow.attr("id").replace("row",""));
        var thisLevel = parseInt(thisRow.children().first().attr("class").replace("textlevel",""));
        var checked = 0;
        var unchecked = 0;
        $(".tablefull tr").each(function(){
            if($(this).attr("class") !== "heading"){
                r = parseInt($(this).attr("id").replace("row",""));
                if(r > thisRowNumber){
                    n = parseInt($(this).children().first().attr("class").replace("textlevel",""));
                    if(n === thisLevel){
                        return false;
                    }
                    if(n > thisLevel){
                        $(this).find("input[name='ci_id']").prop("checked",function(index, oldAttr){
                            if(oldAttr){
                                checked++;
                            }
                            else {
                                unchecked++;
                            }
                            return !oldAttr;
                        });
                    }
                }
            }
        });
        if(checked > 0 && unchecked > 0){
            $(".tablefull tr").each(function(){
                if($(this).attr("class") !== "heading"){
                    r = parseInt($(this).attr("id").replace("row",""));
                    if(r > thisRowNumber){
                        n = parseInt($(this).children().first().attr("class").replace("textlevel",""));
                        if(n === thisLevel){
                            return false;
                        }
                        if(n > thisLevel){
                            $(this).find("input[name='ci_id']").prop("checked",true);
                        }
                    }
                }
            });
        }
    });
});

here's the working code in a fiddle: http://jsfiddle.net/KnU4X/1/

here's the code without my lengthy fix in a fiddle: http://jsfiddle.net/KnU4X/2/

To see the problem, click 'select all' with level 1, then 'select all' with level 2, then 'select all' with level 1 again. You will notice it unchecks everything except the level 2 which becomes checked.

Is there a way to remove all that seemingly redundant code and keep that functionality?

share|improve this question

2 Answers 2

up vote 2 down vote accepted

In general, you would have an easier time doing this if your "nested rows" were actually nested. However - that leads to a variety of other layout problems, etc.

That said - here is a simpler way to accomplish what I think you're looking for:

jQuery code:

$(".checkAll").click(function (e) {
    e.preventDefault();

    // Find the row whose button was clicked.
    var $row = $(this).closest('tr');

    // Capture the associated "level" class.
    var cls = $row.children('td:first-child').attr('class');

    // Create a collection of rows, in which you can store the child-rows
    var $children = $row;

    // Start with the next row, and...
    var $current = $row.next();

    // ... for every subsequent row which is *not* on the same level as the
    // one you started with...
    while ($current.length && ($current.children('td:first-child').attr('class') !== cls)) {
        // Add the row to the collection.
        $children = $children.add($current);
        // Then move on to the next row.
        $current = $current.next();
    }

    // Capture all of the *checkboxes* within the matching rows
    var $checks = $($children).find('input[type="checkbox"]');

    // If ALL checkboxes are checked, uncheck them.
    // Otherwise, check them all.
    if ($checks.length == $checks.filter(':checked').length)
        $checks.prop('checked', false);
    else
        $checks.prop('checked', true);
});

Here is a working jsFiddle showing the code in-action.

share|improve this answer
    
I updated the link - which was accidentally broken. The new link should work correctly. :) –  Troy Alford Apr 24 '13 at 18:55
1  
awesome answer. glad you chased me down from stackoverflow haha. –  Jake Zeitz Apr 24 '13 at 18:59
    
+1 Good one indeed. I was reluctant to rely on class names. –  Terry Young Apr 24 '13 at 19:01
    
Hehe - thanks. :) –  Troy Alford Apr 24 '13 at 19:04
up vote 1 down vote

Interesting scenario.

First an foremost, you're making it harder for yourself by relying on the class name. This forced you to parse class name strings then parse the value to integers.

Hence, I've simply added data-level attributes for each <tr>, signifying their levels. Now you have a value which you do not need to parse.

That is all I've changed in the HTML (which probably means now your HTML has redundant things you can remove, I'll leave that to you)

Next, is the script. Without the need to parse strings and values, you can focus on the logic. IMHO, the code logic is now much more readable and less distracting by all the string/value parsing you've been doing.

DEMO: http://jsfiddle.net/terryyounghk/vdEe8/

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.