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;
});
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');
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;
});
If you love us? You can donate to us via Paypal or buy me a coffee so we can maintain and grow! Thank you!
Donate Us With