HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

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

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
selectionvaryingoptionsdropfirstdownsecondbased

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.getElement

Solution

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.

Basic


etc.

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

}

Code Snippets

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
    }
}
<option value="basic">Basic</option>
function enableBilling(e) {
        document.getElementById('billing-price').disabled=false;
        var planName = plan.options[plan.selectedIndex].value; // ← This
        populateBilling(planName);

}

Context

StackExchange Code Review Q#41814, answer score: 5

Revisions (0)

No revisions yet.