A better looking 'treeview' - dealing with lots of checkboxes

Question

I'm working on the ui for an application which uses a custom ui control that is loosly related to a treeview.

This treeview has two levels:

1. Top level which has a number of different geographical regions as well as a 'select all' item
2. Second level which has a number of countries in those geographical regions

The user is able to use the checkboxes in the top level to select whole regions at once or they can select individual countries by going into any of the regions. Using the 'select all' checkbox allows the user to select everything.

The number of selected countries in each given region is also shown because the countries for only one region can be shown at any one time. The actual number of countries in each of the regions is a lot higher, I cut them out for the purposes of the demo. Most of this code is generated server side but I have full control over the html structure should it need to be changed.

http://jsfiddle.net/HV5Z5/ - is a jsfiddle of it working.

Here is my code:

regionLabelClick = function () {
    $('.vcs-cb-asset-wizard-region:eq(0)').addClass('vcs-cb-asset-wizard-region-selected');

    $('.vcs-cb-asset-wizard-region-select label').click(function () {
        var index = $('.vcs-cb-asset-wizard-region-select label').index($(this));

        $('.vcs-cb-asset-wizard-region').removeClass('vcs-cb-asset-wizard-region-selected');
        $(this).parent('li').addClass('vcs-cb-asset-wizard-region-selected');

        $('.vcs-cb-asset-wizard-regions > div').hide();
        $('.vcs-cb-asset-wizard-regions > div:eq('+ index +')').show();
    });
}

areaSelect = function () {
    // count up how many checkboxes have been checked initially
    areaCount();

    $('.vcs-cb-asset-wizard-region-select-container input').change(function () {
        var val = $(this).val();
        var isChecked = $(this).prop('checked');
        var isMain = false;

        // if the changed checkbox is a top level one
        if ($(this).parent('li').hasClass('vcs-cb-asset-wizard-region')) {
            isMain = true;
        }

        if (isMain) {
            if (isChecked) {
                // if one of the top level checkboxes is checked
                if (val == 0) {
                    // check the 'all regions' one
                    $('.vcs-cb-asset-wizard-choose-region input').prop('checked', true);
                } else {
                    // otherwise check all of the checkboxes associated with the selected region
                    $('.vcs-cb-asset-wizard-regions > div:eq(' + parseInt(val - 1) + ') input').prop('checked', true);
                }
            } else {
                // if one of the top level checkboxes is unchecked
                if (val == 0) {
                    // unchecked 'all regions'
                    $('.vcs-cb-asset-wizard-choose-region input').prop('checked', false);
                } else {
                    // uncheck the checkboxes associated with the unchecked top level checkbox
                    $('.vcs-cb-asset-wizard-regions > div:eq(' + parseInt(val - 1) + ') input').prop('checked', false);
                }
            }
        } else {
            // if one of the second level checkboxes is checked 
            checkCurrentArea($(this), isChecked);
        }

        // has the 'all regions' checkbox been checked?
        if (checkAllSelection()) {
            // check all of the checkboxes
            $('.vcs-cb-asset-wizard-all-regions input').prop('checked', true);
        } else {
            // uncheck all of the checkboxes
            $('.vcs-cb-asset-wizard-all-regions input').prop('checked', false);
        }

        // count up how many checkboxes are now checked
        areaCount();
    });
}

areaCount = function () {

    var totalNumber = 0;
    var numberOfChecked = 0;

    // loop through each of the 5 top levels and count the number of checked second level checkboxes
    // append the values to a span top level <ul>
    $('.vcs-cb-asset-wizard-region-select > li').each(function (i) {
        totalNumber = $('.vcs-cb-asset-wizard-regions > div:eq(' + parseInt(i - 1) + ') input').length;
        numberOfChecked = $('.vcs-cb-asset-wizard-regions > div:eq(' + parseInt(i - 1) + ') input:checked').length;

        $('.vcs-cb-asset-wizard-region-select > li:eq(' + parseInt(i - 1) + ') span').text(numberOfChecked + ' / ' + totalNumber);
    });

}

checkAreaSelection = function (index) {
    var result = true;

    // loop through a given div containing second level checkboxes
    // return false if any of them are unchecked
    $('.vcs-cb-asset-wizard-region-countries:eq(' + index + ') input').each(function () {
        if (!$(this).prop('checked')) {
            result = false;
        }
    });

    return result;
}

checkAllSelection = function () {
    var result = true;

    // loop through the top level checkboxes
    // return false if any of them are unchecked
    $('.vcs-cb-asset-wizard-regions input').each(function () {
        if (!$(this).prop('checked')) {
            result = false;
        }
    });

    return result;
}

checkCurrentArea = function (elem, isChecked) {
    // store index value of the parent div that contains this second level checkbox
    var index = $('.vcs-cb-asset-wizard-region-countries').index(elem.parents('.vcs-cb-asset-wizard-region-countries'));

     // if all of the second level checkboxes in this div are checked
    if (checkAreaSelection(index)) {
        // check the associated top level checkbox
        $('.vcs-cb-asset-wizard-region-select li:eq(' + index + ') input').prop('checked', true);
    } else {
        // uncheck the associated top level checkbox
        $('.vcs-cb-asset-wizard-region-select li:eq(' + index + ') input').prop('checked', false);
    }
}

$(function () {
    regionLabelClick();

    areaSelect();
});

I don't like the messy if statement in my areaSelect() method and there must be a better way to keep track of which checkboxes are checked without having to loop through them all after each one has been changed.

Can anyone offer some pointers as to how the code can be improved?

Answer 1

Your code looks very busy;

if there is one thing you should take away from this review : do not namespace your css classes, it makes for hard to read code.

regionLabelClick

You mention the same CSS class a few times, you should extract those in to a var both for readability and maintainability. You could merge the last two statements since the only difference is eq which is a jQuery function anyway.

I would counterpropose this:

regionLabelClick = function () {
    var cssSelected = 'vcs-cb-asset-wizard-region-selected',
          cssSelectedSelector = '.' + cssSelected,
          cssWizardRegionSelector = '.vcs-cb-asset-wizard-region';
          cssWizardRegionDivSelector = '.vcs-cb-asset-wizard-regions > div';

    $(cssWizardSelector).eq(0).addClass(cssSelected);

    $(cssSelectedSelector + ' label').click(function () {
        var index = $(cssSelectedSelector + ' label').index($(this));

        $(cssWizardSelector).removeClass(cssSelected);
        $(this).parent('li').addClass(cssSelected);

        $( cssWizardRegionDivSelector ).hide().eq(index).show();
    });
};

areaSelect

I agree with you that the code is messy, the root cause, in my mind, is that the selector you use is too generic, and then you need too much code to figure out what specifically was selected and deal with it in a ton of ifs.

I would counter-propose something like

areaSelect = function () {

  // count up how many checkboxes have been checked initially
  areaCount();

      $( selector uniquely identifying the all regions checkbox).change(function(){})
      $( selector identifying the other top level checkboxes   ).change(function(){})
      $( selector identifying the 2nd level checkboxes         ).change(function(){})


}

The looping

You actually only loop over the top-level checkboxes to set the labels ( x/y ) with a smart jQuery to get the count of selected boxes, I like it, not sure how that can be improved.

The rest

The other functions are moderately size and well commented. They could benefit as well from extracting css selector strings into var, but that is optional in my mind.