4
\$\begingroup\$

I have the following code working here on JSFIDDLE:

<!DOCTYPE html>
<html>
<head>
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <script src="//ajax.googleapis.com/ajax/libs/jquery/1.9.1/jquery.min.js"></script>
</head>

<body>

<label for="opt1"><input class="radio-button-selector" type="radio" name="account" id="opt1" value="opt1">Opt1</label>
<label for="opt2"><input class="radio-button-selector" type="radio" name="account" id="opt2" value="opt2">Opt2</label>

<div id="form_select" class="form-group">
     <select id="term" name="term" class="form-control">
          <option value="0">Please select</option>
          <option value="6">6 months</option>
          <option value="9">9 months</option>
          <option value="10">10 months</option>
          <option value="12">12 months</option>
          <option value="18">18 months</option>
          <option value="24">24 months</option>
          <option value="30">30 months</option>
          <option value="36">36 months</option>
          <option value="42">42 months</option>
          <option value="48">48 months</option>
    </select>
</div>

<script type="text/javascript">
if (window.jQuery) {
// jQuery is available.
}
else { console.log("-- no jquery --"); }
</script>

<script type="text/javascript">
$(document).ready(function() {
    $("input.radio-button-selector").change(function(){ 
        console.log("Changed.");
        if ( $(this).val() == "opt2" ) {
            $("#term option[value='48']").hide();
            $("#term option[value='42']").hide();
        }
        else if ( $(this).val() == "opt1" ) {
            $("#term option[value='48']").show();
            $("#term option[value='42']").show();
        }
        });
    });
</script>
</body>
</html>

Questions:

  1. Any way to improve the jQuery code? I saw a lot of times what surprisingly elegant things can be done in JS
  2. Any way to improve the html code?
  3. I only use the class "input-button-selector" for the JavaScript, is this actually needed or could I select the radio buttons in a different way?

Note that this code won't work on IE 8/9/10 (jQuery is not loading)

\$\endgroup\$
  • 3
    \$\begingroup\$ @ProgramFOX If he is not asking for us to fix it, but it works on a major browser (Chrome/FireFox/Safari), then that's okay \$\endgroup\$ – konijn Nov 21 '14 at 17:20
3
\$\begingroup\$

This is a classic example of counting on the UI to keep track of state and model.

Because you know there are only 2 options, and you know how to go from one option to another, you simply hide and add options. Nowhere in your code is explicitly declared what durations are linked to which option.

If you dont care about that, you could optimize the code doing something like this:

$(document).ready(function() {
  var monthsDifference = $("#term option[value='48'], #term option[value='42']");

    $("input.radio-button-selector").change(function(){ 
        if ( $(this).val() == "opt2" ) {
            monthsDifference.hide();
        }
        else if ( $(this).val() == "opt1" ) {
            monthsDifference.show();
        }
        });
    });

I would advise you to care though, and have the code know what the months are and build selectors for each option. Something like this (untested):

$(document).ready(function() {
  var optionMonths = {
      opt1: [6,9,10,12,18,24,30,36,42,48],
      opt2: [6,9,10,12,18,24,30,36]
  }
  //Cache all options
  var $options =  $("#term option");

    $("input.radio-button-selector").change(function(){ 
        //Hide all options first
        $options.hide();    
        //Then show relevant options
        showOptionMonths( optionMonths[ $(this).val()] );
    });
});

function showOptionMonths( months ){
    months.forEach( function( month ){
       $("#term option[value='" + month + "']").show();
    });
}

Of course, I would try to find a better name than opt1 or opt2, but I dont have enough knowledge of what you are trying to achieve to suggest something more meaningful.

Finally, you could cache the selectors for each option, I leave that as an exercise for the reader.

\$\endgroup\$
  • \$\begingroup\$ 1) What is the improvement? Can you explain why is better? 2) Why not test it before posting, as I did post the fiddle, you could just fork it? 3) opt1 and opt2 names are used just for privacy, production code has relevant names. \$\endgroup\$ – bg17aw Nov 22 '14 at 8:00
  • \$\begingroup\$ 4) "This is a classic example of counting on the UI to keep track of state and model." Is this a bad or a good thing? \$\endgroup\$ – bg17aw Nov 22 '14 at 8:05
  • \$\begingroup\$ 5) "Nowhere in your code is explicitly declared what durations are linked to which option." Not sure what you mean and why this is bad. \$\endgroup\$ – bg17aw Nov 22 '14 at 8:09
  • \$\begingroup\$ 6) "you could cache the selectors for each option" I think var monthsDifference = $("#term option[value='48'], #term option[value='42']"); and var $options = $("#term option"); in your code are examples of caching the selectors. Am I wrong? \$\endgroup\$ – bg17aw Nov 22 '14 at 8:32
  • \$\begingroup\$ For the next maintainer it is bad to not have the links between the options and the months. From a software maintenance perspective, the second option is better.But, if you are comfortable by only caching 48 and 42, then you can go for that. \$\endgroup\$ – konijn Nov 22 '14 at 16:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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