Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question

4 Answers 4

up vote 13 down vote accepted

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...
}
share|improve this answer
    
Best answer yet! –  headeronly Jul 11 at 12:55
    
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. –  toto2 Jul 11 at 13:02
    
Amended my answer per @toto2's comment, and added another possible solution. –  Greg Burghardt Jul 11 at 13:15

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();
}
share|improve this answer
1  
These codes much easier to maintain~ –  Firegun Jul 11 at 11:53
5  
This is very clean, but perhaps it's worth explaining to the OP how the "fall-through" switch statement works? –  headeronly Jul 11 at 11:55
    
Thanks,you've made the update faster than me :) –  Eugene T Jul 11 at 12:10
1  
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. –  konijn Jul 11 at 12:55

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;
    }
}
share|improve this answer

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

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.