Animated magazine

How can I write this code better? I'm doing an animated magazine, so I run and then clean the animation per page.

```
magazine.turn({
width: MGZ_WIDTH,
height: MGZ_HEIGHT,
pages:MAGAZINE_PAGES,
display:DISPLAY_PAGE,
acceleration: !isChrome(),
autoCenter: false,
when:{
missing: function (e, pages) {
for (var i = 0; i < pages.length; i++) {
addPage(pages[i], $(this));
}
},
turning: function(e, page, view) {
removeShadow(page);
$(this).turn('page');
Hash.go('page/' + page).update();
removeAnimationCache(page);
},
turned: function(e, page, view) {
addShadow(page);
animatePage(page);
}
}
});

function animatePage(page) {

// var display_value;
// if ( DISPLAY_PAGE === 'double') {
// display_value = 1;
// }
// if ( DISPLAY_PAGE === 'single') {
// display_value = 2;
// }
console.log(page);

if (page===1){

$('.img-logo-an').each(function(i) {
var step = i + 1;
$(this).animate({
opacity: 0.9 }, 1500 * step, function() {
$(this).animate({ opacity: 0}, 3000 * step);
});
});

$('.img-logo-an-2').each(function(i) {
var step = i + 2;
$(this).animate({
opacity: 1 }, 2780 * step, function() {
$(this).animate({ opacity: 0}, 2902 * step);
});
});

$('.p2').find('.car_logo').stop(

I'm not just what would be the best way to refactor your code, but I still have some tips for you. I'll take parts of your code and show you how I would have done using a before/after approach:

Snippet 1

Before

magazine.turn({
                width:  MGZ_WIDTH,
                height: MGZ_HEIGHT,
                pages:MAGAZINE_PAGES,
                display:DISPLAY_PAGE,
                acceleration: !isChrome(),
                autoCenter: false,
                when:{
                    missing: function (e, pages) {
                        for (var i = 0; i < pages.length; i++) {
                            addPage(pages[i], $(this));
                        }
                    },
                    turning: function(e, page, view) {
                        removeShadow(page);
                        $(this).turn('page');
                        Hash.go('page/' + page).update();
                        removeAnimationCache(page);
                    },
                    turned: function(e, page, view) {
                        addShadow(page);
                        animatePage(page);
                    }
                }
            });

After

magazine.turn({
    width:  MGZ_WIDTH,
    height: MGZ_HEIGHT,
    pages:MAGAZINE_PAGES,
    display:DISPLAY_PAGE,
    acceleration: !isChrome(),
    autoCenter: false,
    when:{
        missing: function (e, pages) {
            for (var i = 0; i < pages.length; i++) {
                addPage(pages[i], $(this));
            }
        },
        turning: function(e, page, view) {
            removeShadow(page);
            $(this).turn('page');
            Hash.go('page/' + page).update();
            removeAnimationCache(page);
        },
        turned: function(e, page, view) {
            addShadow(page);
            animatePage(page);
        }
    }
});

What changed

• choose an indentation style and stick with it. Don't mix indentation with tabs and space, don't mix with one tab indentation with two tabs indentation, don't mix 2 spaces indentation with 16 spaces indentation: it's hard to follow your code

Snippet 2

Before

missing: function (e, pages) {
    for (var i = 0; i < pages.length; i++) {
        addPage(pages[i], $(this));
    }
},

After

missing: function (e, pages) {
    var $this = $(this);
    for (var i = 0, len=pages.length; i < len; i++) {
        addPage(pages[i], $this);
    }
},

What changed

• Use a local variable len to cache the pages.length value. This avoid calling pages.length multiples times and improves a bit the for loop performance

• cache jQuery objects and don't call to $(whatever) multiple times. Since you're calling $(this) in the loop, you're calling it multiples times. When you call $(whatever), you're creating a new jQuery object, and this object creation has a non-negligible performance cost for the browser. Avoid calling it multiples times wherever you can. This tip can be applied a lot in your current code (I highlight some places below)

Snippet 3

Before

$('.img-logo-an').each(function(i) {
    var step = i + 1;
    $(this).animate({
        opacity: 0.9 }, 1500 * step, function() {
        $(this).animate({ opacity: 0}, 3000 * step);
    });
});

After

$('.img-logo-an').each(function imgLogoEachLoop(i) {
    function onComplete() {
        $this.animate({opacity: 0}, 3000 * step);
    }

    var step = i + 1;
    var $this = $(this);

    $this.animate({opacity: 0.9}, 1500 * step, onComplete);
});

What changed

• cache jQuery objects as above

• avoid strange code structure and indentation by inlining function as you did. In my example, we can clearly see what are three arguments in this call: $this.animate({opacity: 0.9}, 1500 * step, onComplete);. To me, you're snippet lacks of readability

• declare the internal function onComplete at the begining of the outer function: this avoid strange behavior with JavaScript

• avoid anonymous function to improve debugging experience: this turns errors like Uncaught exception on function '(anonymous function)' into Uncaught exception on function 'imgLogoEachLoop'

Snippet 3

Before

$('.p36').find('.tuum-logo')
        .delay(1000)
        .animo({ animation:'tada' })
        .css({opacity:1});

$('.p36').find('.colgante')
        .animate({opacity:1}, 2300);

After

var TUUMLOGO_SHOW_DELAY = 1000;
var COLGANTE_SHOW_DURATION = 2300;

var $p36 = $('.p36');

$p36.find('.tuum-logo')
    .delay(TUUMLOGO_SHOW_DELAY)
    .animo({ animation:'tada' })
    .css({opacity:1});

$p36.find('.colgante').animate({opacity:1}, COLGANTE_SHOW_DURATION);

What changed

• here again, cache jQuery objects

• don't mix indentation style (I stick with 4 spaces indentation in all my Before examples so I've update your indentation here)

• don't use "magic numbers": 1000 and 2300 were magic numbers to me. Declare some constants early in the code and use those constant when needed. Th

StackExchange Code Review Q#61122, answer score: 6