Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm at a loss as to how I could optimize this bit of code. I realize I could place these objects into some sort of array, but I'm not sure how to go about this.

$(document).ready(function () {
   var $stripe01 = $('.stripe-01');
   var $stripe02 = $('.stripe-02');  
   var $stripe03 = $('.stripe-03');  
   var $stripe04 = $('.stripe-04');  
   var $stripe05 = $('.stripe-05');    
   var $stripe06 = $('.stripe-06');      

$(window).scroll(function () {
    var s = $(this).scrollTop(),
        d = $(document).height(),
        c = $(this).height();

    scrollPercent = (s / (d - c) * 18);
    scrollPercent02 = (s / (d - c) * 14);      
    scrollPercent03 = (s / (d - c) * 30);            
    scrollPercent04 = (s / (d - c) * 4);            

    var position01 = -(scrollPercent * ($(document).width() - $stripe01.width()));
    var position02 = -(scrollPercent02 * ($(document).width() - $stripe01.width()));
    var position03 = -(scrollPercent03 * -($(document).width() - $stripe03.width()));      
    var position04 = (scrollPercent04 * -($(document).width() - $stripe04.width()));      

    $stripe01.css({
        'left': position01
    });
    $stripe02.css({
        'left': position02
    });
    $stripe03.css({
        'right': position03
    });      
    $stripe04.css({
        'right': position03
    });      
    $stripe05.css({
        'right': position03
    });      
    $stripe06.css({
        'left': position04
    });      
});
});
share|improve this question
    
Welcome to Code Review! Your question could be improved by including a short description of what the intended purpose of the code is. –  200_success Jul 3 at 6:21

1 Answer 1

up vote 4 down vote accepted

Basic simplifications

It's good to extract duplicated expressions to a local variable:

scrollPercent = (s / (d - c) * 18);
scrollPercent02 = (s / (d - c) * 14);      
scrollPercent03 = (s / (d - c) * 30);            
scrollPercent04 = (s / (d - c) * 4);

This is equivalent but shorter and safer:

var coef = s / (d - c);
scrollPercent01 = coef * 18;
scrollPercent02 = coef * 14;      
scrollPercent03 = coef * 30;            
scrollPercent04 = coef * 4;    

Similarly:

var position01 = -(scrollPercent * ($(document).width() - $stripe01.width()));
var position02 = -(scrollPercent02 * ($(document).width() - $stripe01.width()));
var position03 = -(scrollPercent03 * -($(document).width() - $stripe03.width()));      
var position04 = (scrollPercent04 * -($(document).width() - $stripe04.width()));

Extract the common $(document).width():

var width = $(document).width();
var position01 = -(scrollPercent * (width - $stripe01.width()));
var position02 = -(scrollPercent02 * (width - $stripe01.width()));
var position03 = -(scrollPercent03 * -(width - $stripe03.width()));      
var position04 = (scrollPercent04 * -(width - $stripe04.width()));      

Btw, the above statements look a bit confusing with the oddly placed negative signs. If I rearrange them, this should be equivalent:

var position01 = -(scrollPercent01 * (width - $stripe01.width()));
var position02 = -(scrollPercent02 * (width - $stripe01.width()));
var position03 =  (scrollPercent03 * (width - $stripe03.width()));      
var position04 = -(scrollPercent04 * (width - $stripe04.width()));      

Using arrays

To convert to using arrays to be able to process with a loop, you would need to create parallel arrays, each with the same number of elements. In this case 6, because that's the most diverse case you need to support. Something like this, but I'm not sure it's really any better or more readable than the original:

var $stripes = [
    $('.stripe-01'),
    $('.stripe-02'),
    $('.stripe-03'),
    $('.stripe-04'),
    $('.stripe-05'),
    $('.stripe-06')
];

$(window).scroll(function () {
    var s = $(this).scrollTop(),
        d = $(document).height(),
        c = $(this).height();

    var coef = s / (d - c);
    var scrollPercents = [
        coef * 18,
        coef * 14,
        coef * 30,
        coef * 4
    ];
    var positions = [
        - (scrollPercents[0] * (width - $stripes[0].width())),
        - (scrollPercents[1] * (width - $stripes[0].width())),
          (scrollPercents[2] * (width - $stripes[2].width())),
          (scrollPercents[2] * (width - $stripes[2].width())),
          (scrollPercents[2] * (width - $stripes[2].width())),
        - (scrollPercents[3] * (width - $stripes[3].width()))
    ];
    var layout_param = [
        'left',
        'left',
        'right',
        'right',
        'right',
        'left'
    ];

    for (var i = 0; i < $stripes.length; ++i) {
        $stripes[i].css({
            layout_param[i]: positions[i]
        });
    }
});
share|improve this answer
    
That was a perfect explanation! Thank you! I will be back to up-vote this when I get the reputation points. I really appreciate it. –  unsider Jul 2 at 21:39

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.