CODEWITHSUNDEEP
javascript
Jason Norris
Jason Norris

Reputation: 47

Reducing code duplication

Is there a better way to do this to reduce code duplication?

Maybe somehow loop the 'pagesTop' array?

The following function is initialized on the windows scroll event.

Thanks.

function redrawSideNav() {
    var pagesTop = new Array();
    $('.page').each(function(index, elem){
        pagesTop[index] = $(this);
    });

    $('.menu, .page').find('a').removeClass('active');
    if ( $(document).scrollTop() >= pagesTop[0].offset().top && $(document).scrollTop() < pagesTop[1].offset().top) {
        var target = '#' + pagesTop[0].attr('id');
        $('[href^="'+target+'"]').addClass('active');
    } else if ( $(document).scrollTop() >= pagesTop[1].offset().top && $(document).scrollTop() < pagesTop[2].offset().top) {
        var target = '#' + pagesTop[1].attr('id');
        $('[href^="'+target+'"]').addClass('active');
    } else if ( $(document).scrollTop() >= pagesTop[2].offset().top && $(document).scrollTop() < pagesTop[3].offset().top) {
        var target = '#' + pagesTop[2].attr('id');
        $('[href^="'+target+'"]').addClass('active');
    }  else if ( $(document).scrollTop() >= pagesTop[3].offset().top && $(document).scrollTop() < pagesTop[4].offset().top) {
        var target = '#' + pagesTop[3].attr('id');
        $('[href^="'+target+'"]').addClass('active');
    }   else if ( $(document).scrollTop() >= pagesTop[4].offset().top) {
        var target = '#' + pagesTop[4].attr('id');
        $('[href^="'+target+'"]').addClass('active');
    } 
}

Upvotes: 2

Views: 149

Answers (3)

Amine Hajyoussef
Amine Hajyoussef

Reputation: 4440

the code below works for dynamic sized arrays:

var scrollTop = $(document).scrollTop();
for (var i=0;i<pagesTop.length ;i++) {
    tempCond = pagesTop[i+1] ? (scrollTop <= pagesTop[i+1].offset().top) : true;
    if(scrollTop >= pagesTop[i].offset().top && tempCond ) {
       var target = '#' + pagesTop[i].attr('id');
       $('[href^="'+target+'"]').addClass('active');    
       break;
    }
}

difference with samurai answer:
rather than adding a fictional item to the array to guarantee the loop is executed at least one time and the second condition of if statement is always true, I check inside the loop if the current element is the last item in the array: if it is , the second condition in the if statement is directly set to true, else, the condition is set to scrollTop <= pagesTop[i+1].offset().top

Upvotes: 1

Sumurai8
Sumurai8

Reputation: 20770

Well, it seems that only the number changes in the following code, so you can use a for-loop with a break-statement. When the break get's triggered, the rest of the for-loop will not be executed. The last else if(...) is the same, but without the last condition. We can fake this by appending a dummy-element to pagesTop, that makes the last condition always true.

var a = {
    offset: function() {
        return {'top': 9999999999999};
    }
};
pagesTop.push( a );

for( var i = 0; i < pagesTop.length-1; i++ ) {
    if ( $(document).scrollTop() >= pagesTop[i].offset().top && $(document).scrollTop() < pagesTop[i+1].offset().top) {
        var target = '#' + pagesTop[i].attr('id');
        $('[href^="'+target+'"]').addClass('active');
        break;
    }
}

Upvotes: 1

micha
micha

Reputation: 49612

Do it step by step:

First I would create a variable for $(document).scrollTop() and replace the code inside the if statements:

var scrollTop = $(document).scrollTop();
var targetIndex = null;

$('.menu, .page').find('a').removeClass('active');
if ( scrollTop >= pagesTop[0].offset().top && scrollTop < pagesTop[1].offset().top) {
    targetIndex = 0
} else if ( scrollTop  >= pagesTop[1].offset().top && scrollTop < pagesTop[2].offset().top) {
    targetIndex = 1;
} else if ( scrollTop  >= pagesTop[2].offset().top && scrollTop < pagesTop[3].offset().top) {
    targetIndex = 2;
}  else if ( scrollTop  >= pagesTop[3].offset().top && scrollTop < pagesTop[4].offset().top) {
    targetIndex = 3;
}   else if ( scrollTop  >= pagesTop[4].offset().top) {
    targetIndex = 4;
} 

var target = '#' + pagesTop[targetIndex].attr('id'); 
$('[href^="'+target+'"]').addClass('active');

After that I would start building a loop instead if of using else if cascades:

var scrollTop = $(document).scrollTop();
var targetIndex = null;
for (var i = 0; i < 4 && !targetindex; i++) {
    if (scrollTop >= pagesTop[i].offset().top && scrollTop < pagesTop[i + 1].offset().top) {
         targetIndex = i;
    }
}
targetIndex = targetIndex || 4;
var target = '#' + pagesTop[targetIndex].attr('id'); 
$('[href^="'+target+'"]').addClass('active');

(untested.. maybe I made some indices/syntax errors..)

Upvotes: 1

Related Questions