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 have two identical objects on the page, so I get more of the same lines of code. Can anything be made simpler, perhaps with merging?

for(var i=0;i<ticks.length,i<ticks2.length;i++) {

  var tick = ticks[i];
  var tick2 = ticks2[i];
  var tickCenter = tick.offsetWidth / 2
  var offset = (i - currentTick) * pixelsPerTick;
  var percent = 1-(Math.abs(i-currentTick)/10);

  tick2.style.top = ((offset + center - tickCenter)|0) + "px";
  tick2.style.opacity = percent;

  tick.style.top = ((offset + center - tickCenter)|0) + "px";
  tick.style.opacity = percent;
}
share|improve this question
    
Extract the redundant code into a function. –  Dagg Jul 4 at 3:53

4 Answers 4

up vote 6 down vote accepted

You can combine the two identical assignments at the end into the same line rather than recalculating the same value again:

for (var i = 0; i < ticks.length && i < ticks2.length; i++) {

    var tick = ticks[i];
    var tick2 = ticks2[i];
    var tickCenter = tick.offsetWidth / 2
    var offset = (i - currentTick) * pixelsPerTick;
    var percent = 1 - (Math.abs(i - currentTick) / 10);

    tick.style.top = tick2.style.top = ((offset + center - tickCenter) | 0) + "px";
    tick.style.opacity = tick2.style.opacity = percent;
}

I also fixed your condition in the for loop so that it safely checks both .length values.

You could also avoid some of the intermediate values, but I personally would preserve them the way you have them because of the readability/understandability advantage of having it this way vs. putting them all in one giant formula.

share|improve this answer

The | 0 trick is a cool way to truncate floats to integers, but it's much less readable and relies on a side-effect for the heavy lifting. Although | 0 is appreciably faster, using Math.floor is preferable since the intent is immediately obvious.

Feel free to disregard this if you've identified this as being a performance bottleneck. If your application is not performing slowly, however, then this may be a case of premature optimisation.

share|improve this answer

I would have gone for this -

var updateStyle = function(items, currentTick, pixelPerTick, center){
  for(i =0; i < items.length; i++){
    var item = items[i];
    var tickCenter = item.offsetWidth / 2
    var offset = (i - currentTick) * pixelsPerTick;
    var percent = 1-(Math.abs(i-currentTick)/10);

    item.style.top = ((offset + center - tickCenter)|0) + "px";
    item.style.opacity = percent;
  }
};


updateStyle(ticks, currentTick, pixelPerTick, center);
updateStyle(ticks2, currentTick, pixelPerTick, center);

Because it is configurable and the method is reusable for any other similar items that might occur in the future. This code keeps the option open for easily using the same for other items.

share|improve this answer

You can also remove all vars and leave just one like this:

for (var i = 0; i < ticks.length && i < ticks2.length; i++) {

    var tick = ticks[i],
        tick2 = ticks2[i],
        tickCenter = tick.offsetWidth / 2,
        offset = (i - currentTick) * pixelsPerTick,
        percent = 1 - (Math.abs(i - currentTick) / 10);

    tick.style.top = tick2.style.top = ((offset + center - tickCenter) | 0) + "px";
    tick.style.opacity = tick2.style.opacity = percent;
}
share|improve this answer
2  
and what exacly is the benefit in doing so? You saved 16 characters.. and lost readability, extensibility and easy changing of the variables you have. I don't think this is a good change.. –  Vogel612 Jul 4 at 8:22

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.