current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

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();
});
share|improve this question
add comment

3 Answers 3

up vote 3 down vote accepted

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'))
     });​​​​​​​​​​​​​​​​​​​

  }

});
share|improve this answer
1  
You can do $('#foo, #bar').each(function(elem() {});, it's better and more concise. –  ANeves Jun 27 '12 at 9:24
    
@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',''))}); –  Jose S Jun 27 '12 at 16:01
add comment
up vote 4 down vote

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.

share|improve this answer
add comment
up vote 1 down vote

@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);
share|improve this answer
add comment

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.