Varying a second drop-down’s options based on a first selection

Posted on

Problem

I have some JavaScript code that allows users to select a plan which triggers the second select box to populate with options. The code works, however, it seems to be very inefficient. Any advice on improving the efficiency?

JavaScript:

var term = 0;
var price = 0;
var additional = 0;
var fix = 0;
var plan = document.getElementById('billing-plan');
plan.addEventListener('change', enableBilling, false);
var select = document.getElementById('billing-price');
select.addEventListener('change', updatePrice, false);

var basic = { 
    "Option" : ["$200/month for 1-yr", "$250/month"],
    "Value" : [300, 350]
}
var prime = { 
    "Option" : ["$300/month for 1-yr", "$350/month"],
    "Value" : [400, 450]
}
var gold = { 
    "Option" : ["$400/month for 1-yr", "$450/month"],
    "Value" : [500, 550]
}

function populateBilling(planName) {
    //RESET BILLING PERIOD OPTIONS AND TOTALS
    select.options.length = 1;
    document.getElementById('payment-total').innerText = 0 + additional;
    document.getElementById('payment-rebill').innerText = 0 + additional;
    //FILL BILLING PERIOD WITH PLAN OPTIONS AND VALUES
    if (planName == "basic") {
         for (i = 0; i < basic.Option.length; i++){
             var temp = document.createElement('option');
             temp.value = basic.Value[i];
             temp.text = basic.Option[i];
             select.appendChild(temp);
         }

    } else if (planName == "prime") {
        for (i = 0; i < basic.Option.length; i++){
            var temp = document.createElement('option');
            temp.value = prime.Value[i];
            temp.text = prime.Option[i];
            select.appendChild(temp);
         }
    } else {
         for (i = 0; i < basic.Option.length; i++){
             var temp = document.createElement('option');
             temp.value = gold.Value[i];
             temp.text = gold.Option[i];
             select.appendChild(temp);
         }
    }
 }

 function enableBilling(e) {
      document.getElementById('billing-price').disabled=false;
      var planName = plan.options[plan.selectedIndex].text.toLowerCase();
      populateBilling(planName);

}

function updatePrice(e) {
      price = parseFloat(select.value);
      fix = 100;
      if (price == select.options[2].value){
          term = 1;
      } 
      document.getElementById('payment-total').innerText = price + additional;
      if (price == select.options[2].value){
      document.getElementById('payment-rebill').innerText = 0 + additional; 
      } else {
      document.getElementById('payment-rebill').innerText = price + additional - fix;
      }
}

Here it is in a jsfiddle: http://jsfiddle.net/aGq9q/13/

Solution

For starters, you can remove a lot of duplicate code from the populateBilling() function like this:

function populateBilling(planName) {
    var options = {
        basic: { 
            "Option" : ["$200/month for 1-yr", "$250/month"],
            "Value" : [300, 350]
        },
        prime: { 
            "Option" : ["$300/month for 1-yr", "$350/month"],
            "Value" : [400, 450]
        },
        gold: { 
            "Option" : ["$400/month for 1-yr", "$450/month"],
            "Value" : [500, 550]
        }
    }

    //RESET BILLING PERIOD OPTIONS
    select.options.length = 1;
    document.getElementById('payment-total').innerText = 0 + additional;
    document.getElementById('payment-rebill').innerText = 0 + additional;
    var data = options[planName];
    if (data) {
        for (var i = 0; i < data.Option.length; i++){
            var temp = document.createElement('option');
            temp.value = data.Value[i];
            temp.text = data.Option[i];
            select.appendChild(temp);
        }
    }
}

Working demo: http://jsfiddle.net/jfriend00/e8629/

Here are a couple ideas when looking to simplify code:

  1. Any time you see the same pattern of code repeated over and over, figure that you should eliminate that repeated code, either with a common function, a loop or some other technique that makes only one copy of common code.

  2. Any time you see an if/else comparing to a bunch of values and then selecting different data based on what matches, consider using the keys of an object to do all the lookup work for you.

  3. Any time you don’t really need data to persist across function calls, consider moving variables inside a function rather than using globals.

  4. Make absolutely sure that all loop variables are declared as local variables. Implicit global variables, particular loop variables are a bad source of hard to find bugs.

Same advice as @jfriend00, with an addition: if the user selects the “Choose Plan” placeholder, disable the billing period selector.

function populateBilling(planName) {
    //RESET BILLING PERIOD OPTIONS
    select.options.length = 1;
    document.getElementById('payment-total').innerText = 0 + additional;
    document.getElementById('payment-rebill').innerText = 0 + additional;
    //FILL BILLING PERIOD WITH PLAN OPTIONS AND VALUES
    var selectedPlan = plans[planName];
    if (selectedPlan) {
        for (var i = 0; i < selectedPlan.Option.length; i++) {
            var temp = document.createElement('option');
            temp.value = selectedPlan.Value[i];
            temp.text = selectedPlan.Option[i];
            select.appendChild(temp);
        }
    } else {
        document.getElementById('billing-price').disabled = true; // ← This
    }
}

Parsing the selection text to get a JavaScript identifier is sketchy. I’d make the value of each option explicit.

<option value="basic">Basic</option>

etc.

function enableBilling(e) {
        document.getElementById('billing-price').disabled=false;
        var planName = plan.options[plan.selectedIndex].value; // ← This
        populateBilling(planName);

}

Leave a Reply

Your email address will not be published. Required fields are marked *