5
\$\begingroup\$

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(true).removeAttr('style');
        }
        if (page === 2 || page === 3) {
            $('.p2').find('.car_logo')
                    .animo( { animation: 'bounceInLeft', duration: 3, timing: 4 } )
                    .css({opacity:1});
        }

        if (page === 16 || page === 17) {
            $('.p16').find('.robo')
                    .animo( { animation: 'bounceInLeft', duration: 2.5 } )
                    .css({opacity:1});
            $('.p17').find('.chic')
                    .animo( { animation: 'bounceInRight', duration: 2.5 } )
                    .css({opacity:1});
        }

        if (page === 24 || page === 25) {
            $('.p25').find('.video-container')
                    .delay(1000)
                    .animate({ opacity:1});
        }
        if (page === 36 || page === 37) {
            $('.p36').find('.tuum-logo')
                    .delay(1000)
                    .animo({ animation:'tada' })
                    .css({opacity:1});              

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

        if (page === 58 || page === 59) {
            $('.p58 .p58-1').delay(1).fadeIn(1);
            $('.p58 .p58-2').delay(200).fadeIn(200);
            $('.p58 .p58-3').delay(350).fadeIn(200);
            $('.p58 .p58-4').delay(500).fadeIn(200);
            $('.p58 .p58-5').delay(650).fadeIn(200);


            $('.p58 .p58-7').delay(650)
                .animate({ opacity: 1}, function() {
                    $('.p58 .p58-8')
                        .animate({ top : 0, opacity: 1 }, 1000, function() {
                            $('.p58 .p58-6').css({opacity: 1});
                        });
            });
            // 
        }


        if (page === 114 || page === 115) {

            $('.p114').find('.letter-1').delay(1600).fadeIn(100);
            $('.p114').find('.letter-2').delay(2000).fadeIn(100);
            $('.p114').find('.letter-3').delay(400).fadeIn(100);
            $('.p114').find('.letter-4').delay(200).fadeIn(100);
            $('.p114').find('.letter-5').delay(1).fadeIn(100);
            $('.p114').find('.letter-6').delay(2300).fadeIn(100);
            $('.p114').find('.letter-7').delay(4500).fadeIn(100);
            $('.p114').find('.letter-8').delay(4000).fadeIn(100);
            $('.p114').find('.letter-9').delay(3800).fadeIn(100);
            $('.p114').find('.letter-10').delay(600).fadeIn(100);
            $('.p114').find('.letter-11').delay(3500).fadeIn(100);
            $('.p114').find('.letter-12').delay(800).fadeIn(100);
            $('.p114').find('.letter-13').delay(3200).fadeIn(100);
            $('.p114').find('.letter-14').delay(1000).fadeIn(100);
            $('.p114').find('.letter-15').delay(1400).fadeIn(100);
            $('.p114').find('.letter-16').delay(3000).fadeIn(100);
            $('.p114').find('.letter-17').delay(1200).fadeIn(100);
            $('.p114').find('.letter-18').delay(600).fadeIn(100);
        }


    } // end animatePage

    function removeAnimationCache(page) {

        if (page === 4) {
            $('.p2').find('.car_logo').stop(true).removeAttr('style');
        }

        if (page === 15 || page === 18) {
            $('.p16').find('.robo')
                    .stop(true)
                    .removeAttr('style');
            $('.p17').find('.chic')
                    .stop(true)
                    .removeAttr('style');
        }

        if (page === 35 || page === 38) {
            $('.p36').find('.tuum-logo')
                    .stop(true)
                    .removeAttr('style');
            $('.p36').find('.colgante')  
                    .stop(true)
                    .removeAttr('style');               
        }

        if (page === 57 || page === 60) {
            $('.p58').find('.p58-img').each(function(){
                $(this).stop(true).removeAttr('style');
            });
            $('.p58').find('.p58-img-animated').each(function(){
                $(this).stop(true).removeAttr('style');
            });

        }

        if (page === 113 || page === 116) {
            $('.p114').find('.letter').each(function(){
                $(this).stop(true).removeAttr('style');
            });

        }

    } // end removeAnimationCache
\$\endgroup\$
6
\$\begingroup\$

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. The next snippet has a better example for this point.

Snippet 4

Before

$('.p58 .p58-1').delay(1).fadeIn(1);
$('.p58 .p58-2').delay(200).fadeIn(200);
$('.p58 .p58-3').delay(350).fadeIn(200);
$('.p58 .p58-4').delay(500).fadeIn(200);
$('.p58 .p58-5').delay(650).fadeIn(200);

After

// declare this early in the code...
var FADEIN_DURATION = 200;
var START_DELAY = 200;
var ADDITIONAL_DELAY = 150;


// ... and this later on
var $p58 = $('.p58');

$p58.find('.p58-1').delay(1).fadeIn(1);
$p58.find('.p58-2').delay(START_DELAY + 0*ADDITIONAL_DELAY).fadeIn(FADEIN_DURATION);
$p58.find('.p58-3').delay(START_DELAY + 1*ADDITIONAL_DELAY).fadeIn(FADEIN_DURATION);
$p58.find('.p58-4').delay(START_DELAY + 2*ADDITIONAL_DELAY).fadeIn(FADEIN_DURATION);
$p58.find('.p58-5').delay(START_DELAY + 3*ADDITIONAL_DELAY).fadeIn(FADEIN_DURATION);

What changed

  • don't use magic numbers: your values 250, 350, 500 and 650 are clearly following the formula 200 + 150x ==> make this appears in your code, and as a bonus it will be easier to switch to a formula like 250 + 320x because you just have to change the constants. In this example the line START_DELAY + 0*ADDITIONAL_DELAY could definitely be written as START_DELAY but I've write it that way to emphasis the formula even more
  • keep jQuery selectors as simple as possible: this code $('.p58').find('.p58-1') performs a bit better than this one $('.p58 .p58-1'). Under the hood, $('.p58') and .find('.p58-1') relies on document.getElementsByClassName browser's native method, whereas $('.p58 .p58-1') needs at first a selector parsing step
  • cache jQuery objects and reuse them. Once you've got the $('.p58') jQuery object, don't recreate it: cache it and reuse it performs better.

Snippet 5

Before

$('.p58 .p58-7').delay(650)
    .animate({ opacity: 1}, function() {
        $('.p58 .p58-8')
            .animate({ top : 0, opacity: 1 }, 1000, function() {
                $('.p58 .p58-6').css({opacity: 1});
            });
});

After

function showP588() {
    $p58.find('.p58-8').animate({ top : 0, opacity: 1 }, 1000, showP586);
}
function showP586() {
    $p58.find('.p58-6').css({opacity: 1});
}

$p58.find('.p58-7').delay(650).animate({ opacity: 1}, showP588);

What changed

  • all the same things are the previous snippet
  • avoid strange code structure and indentation by inlining callback functions in .animate calls. Honestly, which one of those code snippets is easier to read ? This code $p58.find('.p58-7').delay(650).animate({ opacity: 1}, showP588); says: after some delay, animate the .p58-7 element, and when you're done show the P588 element. Having function make it easier to read, as do having a meaningful name. Also, do you know to what this }); corresponds at the end of your snippet ? We have to search for the corresponding brackets, which you shouldn't have to.

Snippet 6

Before

$('.p114').find('.letter-1').delay(1600).fadeIn(100);
$('.p114').find('.letter-2').delay(2000).fadeIn(100);
$('.p114').find('.letter-3').delay(400).fadeIn(100);
$('.p114').find('.letter-4').delay(200).fadeIn(100);
$('.p114').find('.letter-5').delay(1).fadeIn(100);
$('.p114').find('.letter-6').delay(2300).fadeIn(100);
$('.p114').find('.letter-7').delay(4500).fadeIn(100);
$('.p114').find('.letter-8').delay(4000).fadeIn(100);
$('.p114').find('.letter-9').delay(3800).fadeIn(100);
$('.p114').find('.letter-10').delay(600).fadeIn(100);
$('.p114').find('.letter-11').delay(3500).fadeIn(100);
$('.p114').find('.letter-12').delay(800).fadeIn(100);
$('.p114').find('.letter-13').delay(3200).fadeIn(100);
$('.p114').find('.letter-14').delay(1000).fadeIn(100);
$('.p114').find('.letter-15').delay(1400).fadeIn(100);
$('.p114').find('.letter-16').delay(3000).fadeIn(100);
$('.p114').find('.letter-17').delay(1200).fadeIn(100);
$('.p114').find('.letter-18').delay(600).fadeIn(100);

After

var $p14 = $('.p114');

var values = {
    1: {delay: 1600, duration: 100 },
    2: {delay: 2000, duration: 100 },
    3: {delay: 400, duration: 100 },
    4: {delay: 200, duration: 100 },
    5: {delay: 1, duration: 100 },
    6: {delay: 2300, duration: 100 },
    7: {delay: 4500, duration: 100 },
    8: {delay: 4000, duration: 100 },
    9: {delay: 3800, duration: 100 },
    10: {delay: 600, duration: 100 },
    11: {delay: 3500, duration: 100 },
    12: {delay: 800, duration: 100 },
    13: {delay: 3200, duration: 100 },
    14: {delay: 1000, duration: 100 },
    15: {delay: 1400, duration: 100 },
    16: {delay: 3000, duration: 100 },
    17: {delay: 1200, duration: 100 },
    18: {delay: 600, duration: 100 },
};

for (var i in values)
{
    var value = values[i];
    $p14.find('.letter-' + i).delay(value.delay).fadeIn(value.duration);
}

What changed

  • same old tip: cache and reuse jQuery objects
  • I hate doing the same thing again and again so I've put values into an object and I step trough this object to call the fadeIn method. It might be a bit too much here, but this would allow you to externalize or generate those values elsewhere.

Snippet 7

Before

$('.p58').find('.p58-img').each(function(){
    $(this).stop(true).removeAttr('style');
});
$('.p58').find('.p58-img-animated').each(function(){
    $(this).stop(true).removeAttr('style');
});

After

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

$p58.find('.p58-img').stop(true).removeAttr('style');
$p58.find('.p58-img-animated').stop(true).removeAttr('style');

What changed

  • cache and reuse jQuery objects
  • don't call .each whenever it's not needed. Both .stop and .removeAttr work when there is multiple elements in the jQuery object: this means that they're already calling .each under the hood, so you don't need to call it you here. What's strange is that this is exactly what you were doing just before this snippet ($('.p36').find('.tuum-logo').stop(true).removeAttr('style');)

Snippet 8

Before

$('.p36').find('.colgante')
    .stop(true)
    .removeAttr('style');

$('.p58').find('.p58-img-animated').each(function(){
    $(this).stop(true).removeAttr('style');
});

After

$('.p36').find('.colgante')
    .stop(true)
    .removeAttr('style');

$('.p58').find('.p58-img-animated').each(function(){
    $(this)
        .stop(true)
        .removeAttr('style');
});

What changed

  • be constant in your code style. Why the first stop call is on a new line when the second call is not ? Try to choose a style and stick with it, it'll improve global readability of the code

I've highlighted some points here, but please note that I've not fixed all points in all your code. For example, I've shown some places where you can cache jQuery objects, but your code may still contains some others places where this tip is available too. And this is true for all tips.

Here are my two cents for you. Hope this'll help you.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.