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

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

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