jQuery carousel code

I would like feedback on things I can improve. If you want to see the full example in action, here is the link.

```
$(document).ready(function() {

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

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

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

// Assigns the container that has all the sections 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();

// Sets the properties of the arrows that have the events
setCssRect(sliderH, 'w', winWidth);
setCssRect(sliderH, 'h', winHeight);
setCssRect(sliderVMiddle, 'w', winWidth);
setCssRect(sliderVMiddle, 'h', winHeight);
setCssRect(sliderVLast, 'w', winWidth);
setCssRect(sliderVLast, 'h', winHeight);

// sets css size properties on the jQuery element.
function setCssRect(elt, property, value) {
if(property=='w') {
$('> div',elt).css('width', value);
}
else {$('> div',elt).css('height', value);}
}

// Gets the number of Horizontal or Vertical slide
var viewWidth = sliderCountH * winWidth;
var heightVMiddle = sliderCountVMiddle * winHeight;
var heightVLast = sliderCountVLast * winHeight;

// Sets the width and height for the Horizontal and Vertical slides
$('.nav-h').css('width', viewWidth);
$('.nav-v-middle').css('height', heightVMiddle);
$('.nav-v-last').css('height', heightVLast);

var isMobile = {
Android: function() { return navigator.userAgent.match(/Android/i); },
BlackBerry: function() { return navigator.userAgent.match(/BlackBerry/i); },
iOS: function() { return navigator.userAgent.match(/iPh

I took a look at your chat with Tomalak and it seems you got some good advice, so big tip-o'-the-hat to Tomalak.

These are just some thoughts on the overall approach to some of this stuff. I'm not so much looking at your current code, but the things I touch on should be applicable regardless.

As Tomalak mentioned, it's better to avoid sequential numbering. But you've also got the middle and last classes that also implies a sort of numbering system. A numbering system that can't go higher than 3 items in fact, so it's pretty limiting. Remember that the structure of the HTML itself can provide a lot of this stuff; ordering is maintained when you use jQuery to select elements.

To keep the code abstract, I'd probably use group as the name (and class) for the "columns" of slides. While I just used "column" in the description - and you could call them that instead - it locks everything into a concrete idea of how it looks. And that isn't always necessary from a coding perspective.

So, you've got 3 groups, and each has x number of slides. Within each group, you've got a first and a last slide, as determined by their order in the HTML.

Now, if you've got a slide that's marked as, say, "current", navigating to another slide is fairly easy. And you don't need to keep track of the current slide, as you can always find it again by its class name.

Within the group (column), the next and previous slides can be found from the current one using jQuery's aptly named .next() and .prev(). So to go to the next slide (i.e. "down" on your site), you could do something like:

var currentSlide = $(".slide.current"), 
    targetSlide  = currentSlide.next(".slide");
if(targetSlide.length === 0) {
  // no more slides in this group!
} else {
  currentSlide.removeClass("current");
  targetSlide.addClass("current");
}

For moving from group to group, it's a little different. However, provided you want the current functionality of always moving to the first slide in a group, you can do something like this:

var currentSlide = $(".slide.current"), 
    targetSlide  = currentSlide.closest(".group").next(".group").find(".slide:first");
if(targetSlide.length === 0) {
  // no slides to move to!
} else {
  currentSlide.removeClass("current");
  targetSlide.addClass("current");
}

(With your structure you can simply use .parent() instead of .closest(".group"), and .next() without a selector, but I'm being explicit/verbose to make things clearer. Included the pointless if-block for the same reason; you can obviously just do a !== comparison instead)

You'll note that only one line differs between the two, so it's a prime candidate for being factored into a function - or functions, plural.

To keep things fairly abstract, I'd refer to the slide changes as simply "next", "previous", "next group" and "previous group". Only when actually adding the event handlers would I link that to an up/down/left/right direction. Again, it's better, I find, to not tie yourself to a notion of how it looks; the groups might be stacked vertically, with the slides running horizontally, but it wouldn't matter to the code above, since it's more abstract.

In your case, the visual layout does matter in terms of animation (up/down versus left/right), but like with the event handlers, try to make such distinctions "late" in the code if you can.

As for finding a slide's position, you could do something like

var currentSlide = $(".current.slide"),
    slideIndex   = currentSlide.prevAll(".slide").length,
    groupIndex   = currentSlide.closest(".group").prevAll(".group").length;

Anyway, these are just some ideas based on your chat with Tomalak and the site's HTML. There are obviously things that can be optimized (e.g. caching slide/group indices using .data()). Just consider it food for thought.

Here's a simple jsfiddle just to demonstrate the DOM traversal stuff

StackExchange Code Review Q#35925, answer score: 4