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:
-
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.
-
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.
-
Any time you don’t really need data to persist across function calls, consider moving variables inside a function rather than using globals.
-
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);
}