Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

jQuery drying up function

As you can see below, I clearly repeat myself over. I understand that this is bad practice.

So, how can the 4 duplicate lines of code within the if and else statement be refactored into one?

Some guidance toward better practice would be greatly appreciated. Also, any DRY references / tutorials that you found helpful in learning this technique.

$('.inner_wrap .details').click(function() {
    var index = $('.inner_wrap .details').index(this);

    $('.details').removeClass('here');
    $(this).addClass('here');
    $('.view').removeClass('active');
    $(this).find('.view').addClass('active');
    console.log(index);
    if(index > 2){
        index -= 3;
        **// This and its corresponding else statement is the topic of the question**
        $(this).closest('.outer_wrapper').prev('.outer_wrapper').find('.tabSlide').removeClass('tabShow');
        $(this).closest('.outer_wrapper').prev('.outer_wrapper').find('.tabSlide:eq(' + index + ')').addClass('tabShow');
    } else {
        $(this).closest('.outer_wrapper').prev('.outer_wrapper').find('.tabSlide').removeClass('tabShow');
        $(this).closest('.outer_wrapper').prev('.outer_wrapper').find('.tabSlide:eq(' + index + ')').addClass('tabShow');
    }

    return false;
});
like image 609
Seth Avatar asked Jan 15 '23 23:01

Seth


2 Answers

Note that the code is repeated in both if and else, meaning, it's always executed. Just take it out of the statement:

if (index > 2) {
    index -= 3;
}
var elt = $(this).closest('.outer_wrapper').prev('.outer_wrapper');
elt.find('.tabSlide').removeClass('tabShow');
elt.find('.tabSlide:eq(' + index + ')').addClass('tabShow');

Next, note that the jQuery object is just an array. You can simplify your code thus:

if (index > 2) {
    index -= 3;
}
var elt = $(this).closest('.outer_wrapper').prev('.outer_wrapper').find('.tabSlide');
$(elt.removeClass('tabShow')[index]).addClass('tabShow');

Finally, we can eliminate the aux variable, used just to demonstrate how you call the same object:

if (index > 2) {
    index -= 3;
}
$($(this).closest('.outer_wrapper').prev('.outer_wrapper').find('.tabSlide').removeClass('tabShow')[index]).addClass('tabShow');

Please, break this into more than one line of code :D

[EDIT]

OK, and just for fun, here's an even more extreme astronaut-type code, getting rid of the remaining if:

$($(this).closest('.outer_wrapper').prev('.outer_wrapper').find('.tabSlide').removeClass('tabShow')[(index <= 2 ? index : index - 3)]).addClass('tabShow');

BUT! It's awfully unreadable and IMO, you should stick with just the first step. Until it becomes a performance issue, don't overdo it. Applying the DRY rule at the cost of readability/maintainability or just sticking everything into a single line of code makes as much sense as reading & writing minified code. Ie, don't do it :).

[EDIT 2]

@StuartNelson reminded me of the existence of the $.eq() function, which would bring the final code to this (broken into several lines):

$(this).closest('.outer_wrapper')
    .prev('.outer_wrapper')
    .find('.tabSlide')
    .removeClass('tabShow')
    .eq(index <= 2 ? index : index - 3)
    .addClass('tabShow');
like image 119
mingos Avatar answered Jan 17 '23 13:01

mingos


You can modify the index only using the if statement and you could keep a variable for the jQuery element used:

$('.inner_wrap .details').click(function() {
    var index = $('.inner_wrap .details').index(this);

    $('.details').removeClass('here');
    $(this).addClass('here');
    $('.view').removeClass('active');
    $(this).find('.view').addClass('active');
    console.log(index);
    if(index > 2){
       index -= 3;
    }
    var element = $(this).closest('.outer_wrapper').prev('.outer_wrapper');
    element.find('.tabSlide').removeClass('tabShow');
    element.find('.tabSlide:eq(' + index + ')').addClass('tabShow');   
    return false;
});
like image 38
gabitzish Avatar answered Jan 17 '23 13:01

gabitzish