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

Hide / Show panels depending of value of a dropdown

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

Problem

According to the value of a dropdown menu I have to show/hide two panels.
For option 'derive' and 'reject' I should show the comment panel, and just for the 'derive' panel I should also show a derivation panel. There are other

This works, but how could I refactor the code to make it shorter and more efficient without losing clarity?

$("#formTabs").on( "change", "select.actions",function(){

        var parentContainer = $(this).closest("fieldset");

        if(this.value == "reject" || this.value == "derive"){
            parentContainer.find(".commentPanel").show();   
        } 
        // other 3 options where the panel should not show up.
        else { 
            parentContainer.find(".commentPanel").hide();
        }

        // Just for the derive option, a derivation panel must be shown.
        if(this.value == "derive"){ 
            parentContainer.find(".derivationPanel").show();
        } else {
            parentContainer.find(".derivationPanel").hide();            
        }       
    });

Solution

The only "efficiency" improvement I see is caching the result of the this.value == "derive" check to avoid evaluating the string comparison twice. The effect of this is close to nothing,
on the other hand it's good to reduce the number of duplicated literals.

".commentPanel" and ".derivationPanel" are also duplicated literals,
so it would be good to eliminate those too.

Lastly, I would recommend some minor style improvements:

  • Don't put a space between ( "change"



  • Put a space after the comma in "select.actions",function



  • Put a space between ){ in function(){, everywhere



Putting it together:

$("#formTabs").on("change", "select.actions", function() {

    var parentContainer = $(this).closest("fieldset");
    var commentPanel = parentContainer.find(".commentPanel");
    var derivationPanel = parentContainer.find(".derivationPanel");
    var isDerive = this.value == "derive";

    if (this.value == "reject" || isDerive) {
        commentPanel.show();   
    } else { 
        // other 3 options where the panel should not show up.
        commentPanel.hide();
    }

    // Just for the derive option, a derivation panel must be shown.
    if (isDerive) { 
        derivationPanel.show();
    } else {
        derivationPanel.hide();            
    }       
});


Note that by extracting the commentPanel, derivationPanel, isDerive variables you don't lose any efficiency at all, as all the right-hand sides would be evaluated in the code no matter what. This rewritten version is better, because of the string literals now appear only once, which eliminates typos, and if you need to change something, you can do it at a single place instead of two as in the original.

Code Snippets

$("#formTabs").on("change", "select.actions", function() {

    var parentContainer = $(this).closest("fieldset");
    var commentPanel = parentContainer.find(".commentPanel");
    var derivationPanel = parentContainer.find(".derivationPanel");
    var isDerive = this.value == "derive";

    if (this.value == "reject" || isDerive) {
        commentPanel.show();   
    } else { 
        // other 3 options where the panel should not show up.
        commentPanel.hide();
    }

    // Just for the derive option, a derivation panel must be shown.
    if (isDerive) { 
        derivationPanel.show();
    } else {
        derivationPanel.hide();            
    }       
});

Context

StackExchange Code Review Q#68103, answer score: 2

Revisions (0)

No revisions yet.