3
\$\begingroup\$

Here is a JSFiddle of my problem.

I have one <select> on the page that lets you choose a brand:

<select class="form-control" name="website_brand">
    <option value="" disabled selected>Please Select</option>
    <option value="" disabled>────────────────────</option>
    <option value="TEA">Aerospace Testing</option>
    <option value="AIM">Aircraft Interiors</option>
    <option value="TEM">Automotive Testing</option>
    <option value="BAI">Business Airport</option>
    <option value="BJI">Business Jet Interiors</option>
    <option value="EHM">Electric & Hybrid Marine</option>
    <option value="EV">Electric & Hybrid Vehicle</option>
    <option value="ETM">Engine</option>
    <option value="IVM">Industrial Vehicle</option>
    <option value="MCI">Marine Catering</option>
    <option value="MMM">Marine Maintenance</option>
    <option value="MTI">Meteorological</option>
    <option value="PTW">Passenger Terminal World</option>
    <option value="PO">Postal</option>
    <option value="PMC">Professional Motorsport Circuit</option>
    <option value="PMW">Professional Motorsport World</option>
    <option value="STA">Stadia</option>
    <option value="TTM">Tire</option>
    <option value="TFM">Traffic</option>
    <option value="VDM">Vehicle Dynamics</option>
    <option value="VZI">Vision Zero</option>
    <option value="WST">Winter Sports</option>
</select>

And another that displays possible banner types:

<select class="form-control" name="banner_type">
    <option value="" disabled selected>Please Select</option>
    <option value="" disabled>────────────────────</option>
    <option value="9">Central Banner</option>
    <option value="3">Left-Hand Banner</option>
    <option value="5">Right-Hand (Large) Banner</option>
    <option value="7">Right-Hand (Small) Banner</option>
    <option value="1">Top (Header) Banner</option>
    <option value="19">Left-Hand (Large) Banner</option>
    <option value="21">Left-Hand (Small) Banner</option>
    <option value="17">Right-Hand Banner</option>
    <option value="15">Top Banner</option>
    <option value="13">Central Banner</option>
    <option value="11">Top Banner</option>
</select>

And then a huge amount of JavaScript showing and hiding multiple <option> in the second <select> when particular <option> are chosen in the first. Is it possible to condense the following code down in any way?

$(document).ready(function () {
    $("select[name=banner_type] option").hide();
});

$("select[name=website_brand]").click(function () {
    if ($(this).val() == "PTW") {
        // PASSENGER
        $("select[name=banner_type] option[value=15]").show();
        $("select[name=banner_type] option[value=17]").show();
        $("select[name=banner_type] option[value=19]").show();
        $("select[name=banner_type] option[value=21]").show();
        // TRAFFIC
        $("select[name=banner_type] option[value=11]").hide();
        $("select[name=banner_type] option[value=13]").hide();
        // OTHER
        $("select[name=banner_type] option[value=1]").hide();
        $("select[name=banner_type] option[value=3]").hide();
        $("select[name=banner_type] option[value=5]").hide();
        $("select[name=banner_type] option[value=7]").hide();
        $("select[name=banner_type] option[value=9]").hide();
    } else if ($(this).val() == "TFM") {
        // PASSENGER
        $("select[name=banner_type] option[value=15]").hide();
        $("select[name=banner_type] option[value=17]").hide();
        $("select[name=banner_type] option[value=19]").hide();
        $("select[name=banner_type] option[value=21]").hide();
        // TRAFFIC
        $("select[name=banner_type] option[value=11]").show();
        $("select[name=banner_type] option[value=13]").show();
        // OTHER
        $("select[name=banner_type] option[value=1]").hide();
        $("select[name=banner_type] option[value=3]").hide();
        $("select[name=banner_type] option[value=5]").hide();
        $("select[name=banner_type] option[value=7]").hide();
        $("select[name=banner_type] option[value=9]").hide();
    } else {
        // PASSENGER
        $("select[name=banner_type] option[value=15]").hide();
        $("select[name=banner_type] option[value=17]").hide();
        $("select[name=banner_type] option[value=19]").hide();
        $("select[name=banner_type] option[value=21]").hide();
        // TRAFFIC
        $("select[name=banner_type] option[value=11]").hide();
        $("select[name=banner_type] option[value=13]").hide();
        // OTHER
        $("select[name=banner_type] option[value=1]").show();
        $("select[name=banner_type] option[value=3]").show();
        $("select[name=banner_type] option[value=5]").show();
        $("select[name=banner_type] option[value=7]").show();
        $("select[name=banner_type] option[value=9]").show();
    }
});
\$\endgroup\$
6
\$\begingroup\$

Two quick thoughts (and an example jsFiddle):

"switch" will keep you from having to write $(this).val() over and over again (as from the look of it, these lists will continue to grow).

Using $("select[name=banner_type] option").hide(); at the beginning of the .click() will keep you from having to individually .hide() elements.

CODE:

$("select[name=website_brand]").click(function () {
    // hide all 
    $("select[name=banner_type] option").hide();

    switch ($(this).val()){
        case "PTW":
            // show only: PASSENGER
            $("select[name=banner_type] option[value=15]").show();
            $("select[name=banner_type] option[value=17]").show();
            $("select[name=banner_type] option[value=19]").show();
            $("select[name=banner_type] option[value=21]").show();
            break;
        case "TFM":
            // show only: TRAFFIC
            $("select[name=banner_type] option[value=11]").show();
            $("select[name=banner_type] option[value=13]").show();
            break;
        default:
            // show only: OTHER
            $("select[name=banner_type] option[value=1]").show();
            $("select[name=banner_type] option[value=3]").show();
            $("select[name=banner_type] option[value=5]").show();
            $("select[name=banner_type] option[value=7]").show();
            $("select[name=banner_type] option[value=9]").show();
            break;
    }
});

UPDATE: Borrowing the array idea from @Ocelot20, you can further simplify by putting your values in a dictionary and then using a .each() to show the necessary options.

$("select[name=website_brand]").change(function () {
    // hide all 
    $("select[name=banner_type] option").hide();

    var displaySettings = {
        "PTW": [15, 17, 19, 21], // show only: PASSENGER
        "TFM": [11, 13], // show only: TRAFFIC
        "OTHER": [1, 3, 5, 7, 9], // show only: OTHER
    };

    switch ($(this).val()){
        case "PTW":
            jQuery.each(displaySettings["PTW"], function(index, value) {
                $("select[name=banner_type] option[value="+value+"]").show();
            });
            break;
        case "TFM":
            jQuery.each(displaySettings["TFM"], function(index, value) {
                $("select[name=banner_type] option[value="+value+"]").show();
            });
            break;
        default:
            jQuery.each(displaySettings["OTHER"], function(index, value) {
                $("select[name=banner_type] option[value="+value+"]").show();
            });
            break;
    }
});

UPDATE: optimizing for two calls to jQuery (one for hiding, one for showing).

\$\endgroup\$
  • 4
    \$\begingroup\$ This answer is consise and clear, with great ideas that are simple to implement without much more javascript knowledge... and working examples. Thank you! I look forward to playing with it when I reach the office tomorrow! The other answers, whilst look splendid, are far too advanced for me at this stage, but I will endeavour to improve my skills until I reach the stage where I can revisit them. Thanks all. \$\endgroup\$ – mpdc Jun 4 '14 at 19:51
  • \$\begingroup\$ That's exactly what I did, but thought I'd add an explanation to why I accepted that answer too, as well as show a little gratitude? \$\endgroup\$ – mpdc Jun 5 '14 at 5:05
  • \$\begingroup\$ This would be a great answer on stack exchange, but in a code review I am almost tempted to downvote. Since it is the selected answer I want to upvote, that balance earns you a short reprieve ;) This is much clearer/easy-to-read and far more concise, but in JQuery this code has a major performance impact. @Sabe I would suggest that you build the string in your each function and call $() only once per case (see my post in regards to building a selector string). I would definitely upvote that. \$\endgroup\$ – blaze4218 Jun 5 '14 at 14:30
  • \$\begingroup\$ @blaze4218, I understand what you're suggesting (update here), but I don't see the "major performance impact" that you're talking about. I agree that doing a bit of string manipulation is faster than multiple calls to jQuery, but the effect is minimal as far as I can see. That combined with the fact that, as far as the end user is concerned, either way is instantaneous, makes me want to not utilize your suggestion. \$\endgroup\$ – Sabe Jun 5 '14 at 15:49
  • 1
    \$\begingroup\$ For example: In addition to several calls to JQuery, your code can not be later optimized to cache the returned object. By building a single selector string this would be a trivial addition. I always favor improvements when the implementation is trivial and the argument is "why not" codereview.stackexchange.com/questions/52442/… \$\endgroup\$ – blaze4218 Jun 5 '14 at 16:10
3
\$\begingroup\$

A bit short on time, so hopefully you (or someone else) can fill in the blanks here:

  • Instead of just doing this all in a JQuery selector/event function, put this logic in its own method and give it a meaningful name. Something like: toggleBannerTypeVisibility(brandName)
  • Use a table to store all the settings, then write some simple code to loop through it:
var displaySettings = {
    "PTW": [15, 17, 19, 21],
    "TFM": [11, 13]
};
  • Don't toggle in both directions. Keep settings for show or hide only, then either show all and disable selected ones, or vice versa.
  • Use two tiers of settings to make your code more understandable if necessary:
var passengerOptions = [ 15, 17, 19, 21];
var trafficOptions = [11, 13];
// etc.

var displaySettings = {
    "PTW": passengerOptions,
    "TFM": trafficOptions,
    "OtherExample": passengerOptions.concat(trafficOptions)
                                    .concat(otherOptions)
}
\$\endgroup\$
2
\$\begingroup\$

My final answer is at the bottom with a fiddle, but a little background first:

I also think you should use toggle functions instead. It would reduce code repetition.

Perhaps something like

$(document).ready(function () {
    $("select[name=banner_type] option").hide();
});

function OtherShowHide(toggle){
    if (toggle==='show') {  
        $("select[name=banner_type] option[value=1]").show();
        $("select[name=banner_type] option[value=3]").show();
        $("select[name=banner_type] option[value=5]").show();
        $("select[name=banner_type] option[value=7]").show();
        $("select[name=banner_type] option[value=9]").show();
    }
    else {
        $("select[name=banner_type] option[value=1]").hide();
        $("select[name=banner_type] option[value=3]").hide();
        $("select[name=banner_type] option[value=5]").hide();
        $("select[name=banner_type] option[value=7]").hide();
        $("select[name=banner_type] option[value=9]").hide();
    }
}
function TrafficShowHide(toggle){
    if (toggle==='show') {  
        $("select[name=banner_type] option[value=11]").show();
        $("select[name=banner_type] option[value=13]").show();
    }
    else {
        $("select[name=banner_type] option[value=11]").hide();
        $("select[name=banner_type] option[value=13]").hide();
    }
}
function PassengerShowHide(toggle){
    if (toggle==='show') {
        $("select[name=banner_type] option[value=15]").show();
        $("select[name=banner_type] option[value=17]").show();
        $("select[name=banner_type] option[value=19]").show();
        $("select[name=banner_type] option[value=21]").show();
    }
    else {
        $("select[name=banner_type] option[value=15]").hide();
        $("select[name=banner_type] option[value=17]").hide();
        $("select[name=banner_type] option[value=19]").hide();
        $("select[name=banner_type] option[value=21]").hide();
    }
}

$("select[name=website_brand]").click(function () {
    if ($(this).val() == "PTW") {

        // PASSENGER
        PassengerShowHide('show');
        // TRAFFIC
        TrafficShowHide('hide');
        // OTHER
        OtherShowHide('hide');

    } else if ($(this).val() == "TFM") {

        // PASSENGER
        PassengerShowHide('hide');
        // TRAFFIC
        TrafficShowHide('show');
        // OTHER
        OtherShowHide('hide');

    } else {

        // PASSENGER
        PassengerShowHide('hide');
        // TRAFFIC
        TrafficShowHide('hide');
        // OTHER
        OtherShowHide('show');

    }
});

But the question that you asked actually has an answer specific to how JQuery parses the input string to determine selectors. To get the most out of a single call to the JQuery object you can put all of your selectors in a single comma delimited string (if you are performing the same action to all the given selectors). This has the added benefit of much less overhead than calling JQuery many times.

i.e.

$(document).ready(function () {
    $("select[name=banner_type] option").hide();
});

function OtherShowHide(toggle){
    if (toggle==='show') {  
        $("select[name=banner_type] option[value=1] , select[name=banner_type] option[value=3] , select[name=banner_type] option[value=5] , select[name=banner_type] option[value=7] , select[name=banner_type] option[value=9]").show();
    }
    else {
        $("select[name=banner_type] option[value=1] , select[name=banner_type] option[value=3] , select[name=banner_type] option[value=5] , select[name=banner_type] option[value=7] , select[name=banner_type] option[value=9]").hide();
    }
}
function TrafficShowHide(toggle){
    if (toggle==='show') {  
        $("select[name=banner_type] option[value=11] , select[name=banner_type] option[value=13]").show();
    }
    else {
        $("select[name=banner_type] option[value=11] , select[name=banner_type] option[value=13]").hide();
    }
}
function PassengerShowHide(toggle){
    if (toggle==='show') {
        $("select[name=banner_type] option[value=15] , select[name=banner_type] option[value=17] , select[name=banner_type] option[value=19] , select[name=banner_type] option[value=21]").show();
    }
    else {
        $("select[name=banner_type] option[value=15] , select[name=banner_type] option[value=17] , select[name=banner_type] option[value=19] , select[name=banner_type] option[value=21]").hide();
    }
}

$("select[name=website_brand]").click(function () {
    if ($(this).val() == "PTW") {

        // PASSENGER
        PassengerShowHide('show');
        // TRAFFIC
        TrafficShowHide('hide');
        // OTHER
        OtherShowHide('hide');

    } else if ($(this).val() == "TFM") {

        // PASSENGER
        PassengerShowHide('hide');
        // TRAFFIC
        TrafficShowHide('show');
        // OTHER
        OtherShowHide('hide');

    } else {

        // PASSENGER
        PassengerShowHide('hide');
        // TRAFFIC
        TrafficShowHide('hide');
        // OTHER
        OtherShowHide('show');

    }
});

I would further reduce the redundancy found in the strings by declaring them (as if a constant) and passing the variable to the JQuery function.

$(document).ready(function () {
    $("select[name=banner_type] option").hide();
});

function OtherShowHide(toggle){

    var otherOptions = "select[name=banner_type] option[value=1] , " +
                       "select[name=banner_type] option[value=3] , " +
                       "select[name=banner_type] option[value=5] , " +
                       "select[name=banner_type] option[value=7] , " +
                       "select[name=banner_type] option[value=9]";

    if (toggle==='show') {  

        $(otherOptions).show();

    }
    else {

        $(otherOptions).hide();

    }
}

function TrafficShowHide(toggle){

    var trafficOptions = "select[name=banner_type] option[value=11] , " +
                         "select[name=banner_type] option[value=13]";

    if (toggle==='show') {  

        $(trafficOptions).show();

    }
    else {

        $(trafficOptions).hide();

    }

}
function PassengerShowHide(toggle){

    var passengerOptions = "select[name=banner_type] option[value=15] , " +
                           "select[name=banner_type] option[value=17] , " +
                           "select[name=banner_type] option[value=19] , " +
                           "select[name=banner_type] option[value=21]";

    if (toggle==='show') {

        $(passengerOptions).show();

    }
    else {

        $(passengerOptions).hide();

    }
}

$("select[name=website_brand]").click(function () {

    if ($(this).val() == "PTW") {

        // PASSENGER
        PassengerShowHide('show');
        // TRAFFIC
        TrafficShowHide('hide');
        // OTHER
        OtherShowHide('hide');

    } else if ($(this).val() == "TFM") {

        // PASSENGER
        PassengerShowHide('hide');
        // TRAFFIC
        TrafficShowHide('show');
        // OTHER
        OtherShowHide('hide');

    } else {

        // PASSENGER
        PassengerShowHide('hide');
        // TRAFFIC
        TrafficShowHide('hide');
        // OTHER
        OtherShowHide('show');

    }

});

Now when you need to make changes (e.g. adding another option to show when toggling traffic) you only need to make that change in one place. Try it out!

\$\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.