Refactoring multiple `else if` code

Question

I think I can refactor this:

if (clicked_id == "contain-word") {
 $("#specie_filter li.checked").removeClass("checked");
 $("#specie_filter li:first").addClass("checked")
 $("#specie_filter ul").scrollTop(0) 
 $("#cell_filter li.checked").removeClass("checked")
 $("#cell_filter li:first").addClass("checked")
 $("#cell_filter ul").scrollTop(0)
 $("#factor_filter li.checked").removeClass("checked")
 $("#factor_filter li:first").addClass("checked")
 $("#factor_filter ul").scrollTop(0) 
} else if (clicked_id == "specie_filter") {
 $("#cell_filter li.checked").removeClass("checked")
 $("#cell_filter li:first").addClass("checked")
 $("#cell_filter ul").scrollTop(0)
 $("#factor_filter li.checked").removeClass("checked")
 $("#factor_filter li:first").addClass("checked")
 $("#factor_filter ul").scrollTop(0)
} else if (clicked_id == 'cell_filter') {
 $("#factor_filter li.checked").removeClass("checked")
 $("#factor_filter li:first").addClass("checked")
 $("#factor_filter ul").scrollTop(0)
}

it into something like

if (clicked_id == "contain-word" | clicked_id == "specie_filter"`)

but that will make the code more complex and the duplicity still exists in the conditional statement. Does anyone have ideas about refactoring this?

Answer 1

I know I'm late to the game, but each filter function contains essentially the same lines of code. Only the container element differs:

function processFilter($container) {
 $container.find("li.checked").removeClass("checked");
 $container.find("li:first").addClass("checked");
 $container.find("ul").scrollTop(0);
}
switch (clicked_id) {
 case "contain-word":
 processFilter($("#specie_filter"))
 // fall-through...
 case "specie_filter":
 processFilter($("#cell_filter"));
 // fall-through...
 case "cell_filter":
 processFilter($("#factor_filter"));
 // fall-through...
}

EDIT: @toto2, thanks for the reminder. Yeah, comments are useful in this case since omitting the breaks in a switch statement is not normally encountered.

Also, if you want to get a little crazy with the jQuery API, this could also work:

function processFilter(selector) {
 $(selector)
 .find("ul").scrollTop(0)
 .find("li.checked").removeClass("checked")
 .parent()
 .find("li:first").addClass("checked");
}
switch (clicked_id) {
 case "contain-word":
 processFilter("#specie_filter");
 // fall-through...
 case "specie_filter":
 processFilter("#cell_filter");
 // fall-through...
 case "cell_filter":
 processFilter("#factor_filter");
 // fall-through...
}

• linguamachina: Best answer yet!
• toto2: I would add comments in the code where there should be breaks to indicate that they were left out on purpose. Someone else (or your future self) might see this and think it is a bug, and "fix" it.
• Greg Burghardt: Amended my answer per @toto2's comment, and added another possible solution.

Answer 2

This variant came to my mind:

function processFactorFilter() {
 var $factor_filter = $("#factor_filter");
 $("li.checked", $factor_filter).removeClass("checked");
 $("li:first", $factor_filter).addClass ("checked");
 $("ul", $factor_filter).scrollTop(0);
}
function processCellFilter() {
 var $cell_filter = $("#cell_filter");
 $("li.checked", $cell_filter).removeClass("checked");
 $("li:first", $cell_filter).addClass ("checked");
 $("ul", $cell_filter).scrollTop(0);
}
function processSpecieFilter() {
 var $specie_filter = $("#specie_filter");
 $("li.checked", $specie_filter).removeClass("checked");
 $("li:first", $specie_filter).addClass ("checked");
 $("ul", $specie_filter).scrollTop(0);
}
switch (clicked_id) {
 case "contain-word":
 processSpecieFilter();
 // fall-through...
 case "specie_filter":
 processCellFilter();
 // fall-through...
 case "cell_filter":
 processFactorFilter();
}

• Firegun: These codes much easier to maintain~
• linguamachina: This is very clean, but perhaps it's worth explaining to the OP how the "fall-through" switch statement works?
• konijn: I agree with headeronly, while your code could be considered self-explanatory, it is customary on CR to explain your thought process which is as valuable ( or more ? ) than the rewritten code.

Answer 3

So, a different approach is to make this data driven, this way it is extensible with the minimal effort, the code is smaller and easier to read and no duplication remains.

This approach will be slightly slower than a switch statement in some cases - but it doesn't rely on classically confusing features like fall-through.

The function for the loop body is only to help readability.

// this could be one array of 'struct' type objects instead of two...
var ids = [ "contain-word", "specie_filter", "cell_filter" ];
var filters = [ $( "#factor_filter" ), $( "#cell_filter" ), $( "#specie_filter" ) ];
function processFilter( var filter )
{
 $( "li.checked", filter ).removeClass( "checked" );
 $( "li:first", filter ).addClass( "checked" );
 $( "ul", filter ).scrollTop( 0 );
}
for( i = 0; i < ids.size; ++i )
{
 if( ids[ i ] == clicked_id )
 {
 for( j = i; j < ids.size; ++j )
 {
 processFilter( filters[ j ] );
 }
 break;
 }
}

Answer 4

You could shorten the code supplied by Greg

function processFilter($container) {
 $container.find("li.checked, li.first").toggleClass("checked");
 $container.find("ul").scrollTop(0);
}

I like Gregs solution and simply added this is a thrid option to the two he's posted