0
\$\begingroup\$

I am looking to refactor the three lines of code in the else part of the conditional, you'll see where I have it commented.

You'll notice a naming convention for the id's: id, id-div, as seen in the first line: club-community-service, club-community-service-id

I want to shorten that up. I thought maybe storing all names into an array, then looping thru them like (below). If there is a better way to go about this, I am all ears, thanks!

//in theory
array names = [names...];

foreach(names as n) {
  $("#" + n + "-div").toggle($("#" + n).get(0).checked);
}

$('#high-school-student').bind('click', function() {

  if($("input[id=high-school-student]").is(":checked")) {

    $('.field.full.club-community-service, .field.full.other-campus-activities, .field.full.community-public-service, .field.full.other-community-event-activity, .field.honors-program, .field.full.out-of-hs-5-years').hide();

    $('#club-community-service-div, #other-campus-activities-div, #community-public-service-div, #other-community-event-activity-div').hide();

  } else {

    $('.field.full.club-community-service, .field.full.other-campus-activities, .field.full.community-public-service, .field.full.other-community-event-activity, .field.honors-program, .field.full.out-of-hs-5-years').show();

    // ------ RIGHT HERE - best way to refactor these next three lines
    $("#club-community-service-div").toggle($('#club-community-service').get(0).checked);
    $("#other-campus-activities-div").toggle($('#other-campus-activities').get(0).checked);
    $("#community-public-service-div").toggle($('#community-public-service').get(0).checked);
  }
});

$('.watch-for-toggle').bind('click', function() {
  var id = $(this).attr('id');

  $('#' + id + '-div').toggle();
});
\$\endgroup\$
1

3 Answers 3

3
\$\begingroup\$

I didn't figure out a way to shorten those three lines without being too tangled (without modifying the html), but I would rewrite your code in this way:

$('#high-school-student').bind('click', function() {

  var highSchoolStudent = $(this).is(":checked");
  $('.field.full.club-community-service, .field.full.other-campus-activities, .field.full.community-public-service, .field.full.other-community-event-activity, .field.honors-program, .field.full.out-of-hs-5-years').toggle(!highSchoolStudent);

  if (highSchoolStudent) { 

    $('#club-community-service-div, #other-campus-activities-div, #community-public-service-div, #other-community-event-activity-div').hide();

  } else {

     //Use a multiselector, convert the result to array and iterate over it.
     //I used replace to remove the '-div'. A regular expression would be a more elegant solution.
     $.each($('#club-community-service-div, #other-campus-activities-div, #community-public-service-div').toArray(), function(i,v) { 
        $(v).toggle($(v.id.replace('-div', '')).is(':checked'))
     });​​​​​​​​​​​​​​​​​​​

  }

});
\$\endgroup\$
2
  • 1
    \$\begingroup\$ You can do $('#foo, #bar').each(function(elem() {});, it's better and more concise. \$\endgroup\$
    – ANeves
    Commented Jun 27, 2012 at 9:24
  • \$\begingroup\$ @ANeves is right. You can use: $('#club-community-service-div, #other-campus-activities-div, #community-public-service-div').each(function(i,elem) {$(elem).toggle(elem.id.replace('-div',''))}); \$\endgroup\$
    – Jose S
    Commented Jun 27, 2012 at 16:01
4
\$\begingroup\$

If you have control over the div tags and can make the html something like this:

<div id='club-community-service-div' data-rel='club-community-service' class='check-toggle'>

then you can do something like this:

$('.check-toggle').toggle(function () { return document.getElementById($(this).data('rel')).checked; });

However, a much better change to this code would be to cache the various DOM traversals.

\$\endgroup\$
1
\$\begingroup\$

@BillBarry proposes a good way to address your problem.

Other improvements

  1. Write $("input#high-school-student") instead of $("input[id=high-school-student]");
  2. But in your particular use, write $(this), since the element is the one raising the event;
  3. Use JQuery 1.7's .on() instead of .bind();
  4. What if the checkbox changes without being clicked? You should bind to other events as well: input (mind IE!), change, etc.
  5. You can use .toggle(bool) to show/hide the element list, instead of doing the if and repeating the selector;
  6. You should cache and re-use the JQuery DOM elements, where possible, instead of repeatedly getting them (but avoid polluting the global scope);
  7. Naming an id foo-div raises alarm bells, it's "hungarian notation" for HTML! Why aren't you just targeting #foo > div or #foo div instead?? Even if you have other divs in there, you can give it a class - #foo > .bar. Or if it's adjacent, #foo + div. Etc.;

5:

var isHighSchoolStudent = $(this).is(':checked');
$('.field.full.club-community-service, .field.full.other-campus-activities, .field.full.community-public-service, .field.full.other-community-event-activity, .field.honors-program, .field.full.out-of-hs-5-years'
    ).toggle(!isHighSchoolStudent);
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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