5
\$\begingroup\$

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/

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the response! I was told that is bad habit to do DOM manipulation inside of a loop. Having said that, any recommendations for appending the drop-downs outside the loop? \$\endgroup\$ – Ryan Salmons Feb 16 '14 at 18:04
  • \$\begingroup\$ @RyanSalmons - it depends upon what you're doing in the loop. There's no issue with adding two option values inside a loop here. In this case it's the more efficient way of doing it. \$\endgroup\$ – jfriend00 Feb 16 '14 at 18:10
6
\$\begingroup\$

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);

}
\$\endgroup\$

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.