Hide / Show panels depending of value of a dropdown

Question

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

Answer 1

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.

• Alejandro Veltri: In the case of isDerive, declaring that extra variable does not produce any performance drawbacks? If not, why?
• janos: It's the opposite: it's the only thing in the above code that improves performance. Thanks to it, JavaScript can evaluate the expression this.value == "derive" once instead of twice. So one string comparison instead of 2 string comparisons.
• Alejandro Veltri: Ok, but in the case that ´isDerive´ was evaluated just once in the code, the performance would be worst when declaring and assigning the value compared with evaluating in just inside the if statement? o declaring variables is near to costless?
• janos: The cost of a single (small) variable + 1 string comparison is surely smaller than the cost of no variables but 2 string comparisons.

Answer 2

If the panels are never dynamically recreated then you can move the code to find them outside the event handler so that you do not need to traverse the DOM each time the event is called.

You can also make the code shorter by using .toggle() rather than .show() and .hide().

var formTab = $("#formTabs");
var fieldset = formTab.closest("fieldset");
var comments = fieldset.find(".commentPanel");
var derivations = fieldset.find(".derivationPanel");
formTab.on("change", "select.actions", function(){
 comments.toggle(this.value == "reject" || this.value == "derive");
 derivations.toggle(this.value == "derive");
 });