2
\$\begingroup\$

I've just finished a program for a 2-way carousel. Meaning, if I click left or right, you are able to go up and down, depending on where you are. This is suppose to work for iPad or tablet only.

I would like to know how to make the code better or if there are any suggestions. I would appreciate it. I am new at this, so I've done my best.

$(document).ready(function() {

    // Find matches
var mql = window.matchMedia("(orientation: portrait)");

var all = document.getElementById("slider-view");

var winWidth            = 0;
var winHeight           = 0;    

    winWidth            = window.innerHeight;
    winHeight           = window.innerWidth;        

// If there are matches, we're in portrait
if(mql.matches) {  
    // Portrait orientation
    $(all).addClass('portrait-transform');

    $(all).css('height', window.innerWidth);
    $(all).css('width', window.innerHeight);
    $(all).css('top', (window.innerHeight - window.innerWidth) / 2);
    $(all).css('left', (window.innerWidth - window.innerHeight) / 2);

    winWidth            = window.innerHeight;
    winHeight           = window.innerWidth; 

    console.log('height '+ $(all).css('height')+' width '+$(all).css('width')+' top '+$(all).css('top')+' left '+$(all).css('left'));                   

} else {  
    // Landscape orientation
    $('#slider-view').removeClass('portrait-transform');  

    all.style.height = window.innerHeight + "px";
    all.style.width = window.innerWidth + "px";

    winWidth            = window.innerWidth;
    winHeight           = window.innerHeight;

    console.log('height '+ $(all).css('height')+' width '+$(all).css('width')+' top '+$(all).css('top')+' left '+$(all).css('left'));                   

}

// Add a media query change listener
mql.addListener(function(m) {
    if(m.matches) {
        // Changed to portrait
        $(all).addClass('portrait-transform');

        $(all).css('height', window.innerWidth);
        $(all).css('width', window.innerHeight);
        $(all).css('top', (window.innerHeight - window.innerWidth) / 2);
        $(all).css('left', (window.innerwidth - window.innerheight) / 2);

        console.log('height '+ $(all).css('height')+' width '+$(all).css('width')+' top '+$(all).css('top')+' left '+$(all).css('left'));                   


    }
    else {
        // Changed to landscape
        $(all).removeClass('portrait-transform');

        $(all).css('height', window.innerWidth);
        $(all).css('width', window.innerHeight);
        $(all).css('top', 0);
        $(all).css('left', 0);          

        var winWidth            = window.innerWidth;
        var winHeight           = window.innerHeight;

        console.log('height '+ $(all).css('height')+' width '+$(all).css('width')+' top '+$(all).css('top')+' left '+$(all).css('left'));                   
    }
});


var sliderWidth = window.innerWidth;
var sliderHeight = window.innerHeight;

// Assigns the container that has all the sectios that will be scrolled horizontally    
var sliderH             = $('.nav-h'); 
var sliderVMiddle       = $('.nav-v-middle');
var sliderVLast         = $('.nav-v-last');

// Gets the number of slides of the horizontal slider
var sliderCountH        = $('.nav-h').children().size(); 
var sliderCountVMiddle  = $('.nav-v-middle').children().size(); 
var sliderCountVLast    = $('.nav-v-last').children().size(); 

// assign width and height to the main scrollers
$('.nav-h > div').css('width', winWidth); // Asigns the width to the view
$('.nav-h > div').css('height', winHeight); // Asigns the height to the view 
$('.nav-v-middle > div').css('width', winWidth); // Asigns the width to the view
$('.nav-v-middle > div').css('height', winHeight); // Asigns the height to the view 
$('.nav-v-last > div').css('width', winWidth); // Asigns the width to the view
$('.nav-v-last > div').css('height', winHeight); // Asigns the height to the view 

var viewWidth           = sliderCountH * winWidth; 
var heightVMiddle       = sliderCountVMiddle * winHeight;
var heightVLast         = sliderCountVLast * winHeight;

$('.nav-h').css('width', viewWidth); // assigns the width
$('.nav-v-middle').css('height', heightVMiddle); // assigns the width
$('.nav-v-last').css('height', heightVLast); // assigns the width

var viewSliderH         = $('#slider-view').css('width', winWidth); // Asigns the width to the view
var viewSliderV         = $('#slider-view').css('height', winHeight); // Asigns the height to the view    



var isMobile = {
    Android: function() { return navigator.userAgent.match(/Android/i); },
    BlackBerry: function() { return navigator.userAgent.match(/BlackBerry/i); },
    iOS: function() { return navigator.userAgent.match(/iPhone|iPad|iPod/i); },
    Opera: function() { return navigator.userAgent.match(/Opera Mini/i); },
    Windows: function() { return navigator.userAgent.match(/IEMobile/i); },
    any: function() { return (isMobile.Android() || isMobile.BlackBerry() || isMobile.iOS() || isMobile.Opera() || isMobile.Windows()); }
};                                  

if ( isMobile.any() ) {

    $('a#prevh').toggle();
    $('a#nexth').toggle();  
    $('a#prevv').toggle();
    $('a#nextv').toggle();  

    var horizontalIndex = 0;
    var verticalIndexMiddle = 0;
    var verticalIndexLast = 0;

    // actions for the swiperight
    $(sliderH).hammer({prevent_default:true}).on('swiperight', function() {
        if(verticalIndexMiddle!==0) {
            $('> div', sliderVMiddle).animate({
                top: '+=' + (sliderHeight*verticalIndexMiddle)
            }, 400);
            verticalIndexMiddle = 0;                        
        }
        if(verticalIndexLast!==0) {
            $('> div', sliderVLast).animate({
                top: '+=' + (sliderHeight*verticalIndexLast)
            }, 400); 
            verticalIndexLast = 0;                      
        }           
        if(horizontalIndex!==0){
            $('> div', sliderH).animate({
                left: '+=' + sliderWidth
            }, 400);
            horizontalIndex-=1;
        }
    });    


    // Actions for the swipeLeft
    $(sliderH).hammer({prevent_default:true}).on('swipeleft', function() {  
        if(verticalIndexMiddle!==0) {
            $('> div', sliderVMiddle).animate({
                top: '+=' + (sliderHeight*verticalIndexMiddle)
            }, 400); 
            verticalIndexMiddle = 0;                    
        } 
        if(verticalIndexLast!==0) {
            $('> div', sliderVLast).animate({
                top: '+=' + (sliderHeight*verticalIndexLast)
            }, 400);
            verticalIndexLast = 0;                      
        }                           
        if(horizontalIndex!==sliderCountH-1){
            $('> div', sliderH).animate({
                left: '-=' + sliderWidth
            }, 400);
            horizontalIndex+=1;
        }  
    }); 


    // Actions for the swipeUp
   $(sliderH).hammer({prevent_default:true}).on('swipedown', function() {   
        if(horizontalIndex==1) {
            if(verticalIndexMiddle!==0) {   
                $('> div', sliderVMiddle).animate({
                    'top': '+=' + sliderHeight
                }, 400); 
                verticalIndexMiddle-=1;
            }
        }
        else if (horizontalIndex==2) {
            if(verticalIndexLast!==sliderCountVLast-1) {    
                $('> div', sliderVMiddle).animate({
                    'top': '+=' + sliderHeight
                }, 400); 
                verticalIndexLast-=1;
            }               
        }
   });

    // Actions for the swipeDown
    $(sliderH).hammer({prevent_default:true}).on('swipeup', function() {    
        if(horizontalIndex==1) { 
            if(verticalIndexMiddle!==sliderCountVMiddle-1) {        
                $('> div', sliderVMiddle).animate({
                    top: '-=' + sliderHeight
                }, 600);
                verticalIndexMiddle+=1;
            }
        }
        else if (horizontalIndex==2) {
            if(verticalIndexLast!==sliderCountVLast-1) {        
                $('> div', sliderVLast).animate({
                    top: '-=' + sliderHeight
                }, 600);
                verticalIndexLast+=1;
            }               
        }
   });     


}

else { 

    var horizontalIndex = 0;
    var verticalIndexMiddle = 0;
    var verticalIndexLast = 0;

   $('a#prevh').on('click', function(event) {
        if(verticalIndexMiddle!==0) {
            $('> div', sliderVMiddle).animate({
                top: '+=' + (sliderHeight*verticalIndexMiddle)
            }, 600);
            verticalIndexMiddle = 0;                        
        }
        if(verticalIndexLast!==0) {
            $('> div', sliderVLast).animate({
                top: '+=' + (sliderHeight*verticalIndexLast)
            }, 600); 
            verticalIndexLast = 0;                      
        }               
        if(horizontalIndex!==0) {   
            $('> div', sliderH).animate({
                left: '+=' + sliderWidth
            }, 600); 
            horizontalIndex-=1;
        }
        console.log('horizontal '+horizontalIndex+' vertical1 '+verticalIndexMiddle+' vertical2 '+verticalIndexLast);       
   });

   $('a#nexth').on('click', function(event) { 
        if(verticalIndexMiddle!==0) {
            $('> div', sliderVMiddle).animate({
                top: '+=' + (sliderHeight*verticalIndexMiddle)
            }, 600); 
            verticalIndexMiddle = 0;                    
        } 
        if(verticalIndexLast!==0) {
            $('> div', sliderVLast).animate({
                top: '+=' + (sliderHeight*verticalIndexLast)
            }, 600);
            verticalIndexLast = 0;                      
        }                
        if(horizontalIndex!==sliderCountH-1) {      
            $('> div', sliderH).animate({
                left: '-=' + sliderWidth
            }, 600);
            horizontalIndex+=1;
        }
        console.log('horizontal '+horizontalIndex+' vertical1 '+verticalIndexMiddle+' vertical2 '+verticalIndexLast);   
   }); 

   $('a#prevv').on('click', function(event) {
        if(horizontalIndex==1) {
            if(verticalIndexMiddle!==0) {   
                $('> div', sliderVMiddle).animate({
                    top: '+=' + sliderHeight
                }, 600); 
                verticalIndexMiddle-=1;
            }
        }
        else if (horizontalIndex==2) {
            if(verticalIndexLast!==sliderCountVLast-1) {    
                $('> div', sliderVMiddle).animate({
                    top: '+=' + sliderHeight
                }, 600); 
                verticalIndexLast-=1;
            }               
        }
        console.log('horizontal '+horizontalIndex+' vertical1 '+verticalIndexMiddle+' vertical2 '+verticalIndexLast);       
   });

   $('a#nextv').on('click', function(event) {  
        if(horizontalIndex==1) { 
            if(verticalIndexMiddle!==sliderCountVMiddle-1) {        
                $('> div', sliderVMiddle).animate({
                    top: '-=' + sliderHeight
                }, 600);
                verticalIndexMiddle+=1;
            }
        }
        else if (horizontalIndex==2) {
            if(verticalIndexLast!==sliderCountVLast-1) {        
                $('> div', sliderVLast).animate({
                    top: '-=' + sliderHeight
                }, 600);
                verticalIndexLast+=1;
            }               
        }
        console.log('horizontal '+horizontalIndex+' vertical1 '+verticalIndexMiddle+' vertical2 '+verticalIndexLast);   
   });       


} 


});
\$\endgroup\$
2
\$\begingroup\$

There's a lot of code there, so allow me to generalize a bit rather than go line by line.

I see some overall issues with your code:

  1. There's a lot of repetition, redundancy, and hard-coded values
  2. You're mixing jQuery and raw DOM pretty randomly instead of sticking to one or the other
  3. You're not using jQuery very efficiently (many lines can be combined)
  4. I get the feeling you're not using stylesheets efficiently either, considering all the styling you're doing in JavaScript

That last bit is difficult to judge without seeing everything in action, but call it a gut feeling.

For the rest, here are some examples:

jQuery or native DOM?

You start out by getting the slider-view element using document.getElementById() instead of just using jQuery's $(). For the rest of the code, you then call $(all) to wrap that element again and again.

Instead you can just say var all = $('#slider-view'), and you won't have to use $(all) anywhere; all will already be a jQuery object.

More efficient jQuery

You should combine more of your expressions, rather than doing just one thing per line. For instance (assuming all = $('#slider-view')), the first if-block can be written as:

all.addClass('portrait-transform').css({
  height: window.innerWidth,
  width:  window.innerHeight,
  top:    (window.innerHeight - window.innerWidth) / 2,
  left:   (window.innerWidth - window.innerHeight) / 2
});

and similarly for the else block, and many other places in the code.

I should add that jQuery also supports relative animation values, which could be beneficial here. For instance, to move something 50 pixels right, you can use .animate({left: "+=50"}) instead of calculating an absolute value.
I'm not saying you must use that feature of jQuery, but it's just to mention that it's there.

DRY - Don't repeat yourself

With regard to repetition and redundancy, you're repeating a lot of code for mobile/non-mobile.
As far as I can tell, you want your slides to move up, down, left or right. The code for doing that is identical regardless of how the move was triggered. Hence, you should abstract that into functions.

For instance, make slideUp(), slideDown(), slideLeft() and slideRight() functions and let them figure out how much to move things for mobile/non-mobile. That way, the non-mobile event handlers could look like this:

$('a#prevh').on('click', slideRight);

for non-mobile, and like this:

sliderH.hammer({prevent_default:true}).on('swiperight', slideRight);

for mobile.

All your code for sliding things around would be in one place and easier to maintain. And while you're at it, you might want to move the 600/400 pixel values into variables, so you only have to write (or calculate) those in one place.

Of course moving a slide up is basically the same as moving it down (as is left vs right); the same calculations, just multiplied by -1 or +1. So that can no doubt be generalized and refactored further. Be sure to have a look around this site; there have been plenty of code reviews of slideshow/carousel code, and they might give you some ideas.


There's more, but the above are the most pressing concerns in terms of structure. Fixing the big stuff will make it easier to focus on the details.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks a lot! I will make these changes and later I will publish again to keep polishing my code! \$\endgroup\$ – Monica Nov 18 '13 at 2:03
  • \$\begingroup\$ @Monica No problem. However, be sure to post new code as a new question. If you just update a question, existing answers will lose their context. So be sure to mark this current question as answered by clicking a checkmark somewhere, and create a new one if you have new code you want reviewed. \$\endgroup\$ – Flambino Nov 18 '13 at 3:18
  • \$\begingroup\$ Thank you as soon as make the corrections, I'll do that. I am pretty sure more stuff will come up. \$\endgroup\$ – Monica Nov 19 '13 at 2:06
1
\$\begingroup\$

By order of importance :
- You can do quite a lot of code factorization.
- You should comment more
- You might choose here and there more clear names.

Rq : The code below does not handle the 600 vs 400 issue(you must defines those figures in some var, then you'll figure out the changes)

But this code should be more easy to maintain once you added some comments to it, since it holds in 160 lines vs your 310 lines, and does things only once.

The idea - that you can really push further than i did - is to have functional blocks easy to understand, written from higher importance to lower (utilities are at the end).

$(document).ready(function () {

var all = $("#slider-view");

var winWidth = window.innerHeight;
var winHeight = window.innerWidth;

handlePortraitOrLandscape();

// ---------------------------------- 
//     general element size setup

var sliderWidth = winWidth; // why renaming ?
var sliderHeight = winHeight; // why renaming ?

// Assigns the container that has all the sectios that will be scrolled horizontally    
var sliderH = $('.nav-h');
var sliderVMiddle = $('.nav-v-middle');
var sliderVLast = $('.nav-v-last');

// Gets the number of slides of the horizontal slider
var sliderCountH = sliderH.children().size();
var sliderCountVMiddle = sliderVMiddle.children().size();
var sliderCountVLast = sliderVLast.children().size();

// assign width and height to the main scrollers
setCssRect(sliderH, sliderCountH * winWidth, winHeight);
setCssRect(sliderVMiddle, sliderCountVMiddle * winHeight, winHeight);
setCssRect(sliderVLast, winWidth, sliderCountVLast * winHeight);

//    var viewSliderH, viewSliderV are not used, and width / height allready set in HandlePortraitOrLandscape()  

var horizontalIndex = 0;
var verticalIndexMiddle = 0;
var verticalIndexLast = 0;

// -----------------------------
// event handlers hooking

if (isMobile()) {
    // *** do those four changes at once with a class    
    $('a#prevh').toggle();
    $('a#nexth').toggle();
    $('a#prevv').toggle();
    $('a#nextv').toggle();

    var hammer = $(sliderH).hammer({  prevent_default: true   });

    // Comment here
    hammer.on('swiperight', swipeRight);
    hammer.on('swipeleft', swipeLeft);
    hammer.on('swipedown', swipeDown);
    hammer.on('swipeup', swipeUp);
} else {
    // Comment here
    $('a#prevh').on('click', swipeRight);
    $('a#nexth').on('click', swipeLeft);
    $('a#prevv').on('click', swipeDown);
    $('a#nextv').on('click', swipeUp);
}

 // -----------------------------
 // event handlers

function swipeRight() {
    findMeAGoodHorizontalName(sliderCountH - 1, -1);
}

function swipeLeft() {
    findMeAGoodHorizontalName(0, 1);
}

function swipeDown() {
    findMeAGoodVerticalName(0, sliderCountVLast - 1, sliderVMiddle, sliderVMiddle, -1);
}

function swipeUp() {
    findMeAGoodVerticalName(sliderCountVMiddle - 1, sliderCountVLast - 1, sliderVMiddle, sliderVLast, +1);
}

// -----------------------------
// slider animation functions

function findMeAGoodHorizontalName(hLimit, hShift) {
    if (verticalIndexMiddle !== 0) {
        animateDivTop(sliderVMiddle, sliderHeight * verticalIndexMiddle);
        verticalIndexMiddle = 0;
    }
    if (verticalIndexLast !== 0) {
        animateDivTop(sliderVLast, sliderHeight * verticalIndexLast);
        verticalIndexLast = 0;    
    }
    if (horizontalIndex != hLimit) {
        animateDivLeft(sliderH, sliderWidth * hShift);
        horizontalIndex += hShift;
    }
}

function findMeAGoodVerticalName(verticalLimit1, verticalLimit2, animatedSlider1, animatedSlider2, vShift) {
    if (horizontalIndex == 1) {
        if (verticalIndexMiddle !== verticalLimit1) {
            animateDivTop(animatedSlider1, vShift * sliderHeight);
            verticalIndexMiddle += vShift;
        }
    } else if (horizontalIndex == 2) {
        if (verticalIndexLast !== verticalLimit2) {
            animateDivTop(animatedSlider2, vShift * sliderHeight);
            verticalIndexLast += vShift;
        }
    }
}

// -----------------------------
// css helpers 

function animateDivTop(elt, increase) {
    $('> div', elt).animate({
        top: '+=' + increase
    }, 400);
}

function animateDivLeft(elt, increase) {
    $('> div', elt).animate({
        left: '+=' + increase
    }, 400);
}

// sets css size properties on the jQuery element.
// works with width, height OR width, height, top, left.
function setCssRect(elt, width, height, top, left) {
    elt.css('width', width);
    elt.css('height', height);
    if (arguments.length > 2) {
        elt.css('top', top);
        elt.css('left', left);
    }
}

// some comment here
function handlePortraitOrLandscape() {
    // Find matches // rewrite comment.
    var mql = window.matchMedia("(orientation: portrait)");

    // If there are matches, we're in portrait
    setupPortaitOrLandscape(mql.matches);

    // Add a media query change listener
    mql.addListener(function (m) {
        setupPortaitOrLandscape(m.matches);
    });

    function setupPortaitOrLandscape(isPortrait) {
        if (isPortrait) setupPortraitMode();
        else setupLandscapeMode();
    }

    function setupPortraitMode() {
        all.addClass('portrait-transform');
        setCssRect(all, winHeight, winWidth, (winHeight - winWidth) / 2, (winWidth - winHeight) / 2);
    }

    function setupLandscapeMode() {
        all.removeClass('portrait-transform');
        setCssRect(all, winWidth, winHeight, 0, 0);
    }
}

// -----------------------------
//  UTILITY 

var mobileTests = {          //easier to update
    Android: /Android/i,
    BlackBerry: /BlackBerry/i,
    iOS: /iPhone|iPad|iPod/i,
    Opera: /Opera Mini/i,
    Windows: /IEMobile/i,
};

//  Detect if browser is a mobile browser.
function isMobile() {
    for (var tst in mobileTests) {
        if (navigator.userAgent.match(tst)) return true;
    }
    return false;
};

});
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Oh WOW this is great! I will totally make these changes. Thank you very much, just by looking at this I am learning so much about the mistakes I am making \$\endgroup\$ – Monica Nov 18 '13 at 2:17
  • \$\begingroup\$ You're welcome ! i had forgotten to reset verticalIndexLast in 'findMeAGoodHorizontalName', it's corrected. I think you should always wonder, when copy pasting some code of yours, if you shouldn't rather build a function (or a class) that does that task. Less work, less bugs ! \$\endgroup\$ – GameAlchemist Nov 18 '13 at 8:24

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.