Carousel for website

I have been building my own carousel over the past few days. I am not a jQuery guru, just an enthusiast, and I think my code is a little sloppy, hence the post.

Basically what happens is:

• If someone hits the page with a # values in the URL it will show the appropriate slide and example would be www.hello.com#two, this would slide to slide two.

• If someone clicks the numbers it will show the appropriate slide.

• Next and prev also slide through the slides.

Is there anything I could have written better, as I know there is a lot of duplicate code?

if ($slideNumber == 1) {
    $('#prev').attr("class", "not_active")
    $('#next').attr("class", "active")
}
else if ($slideNumber == divSum) {
    $('#next').attr("class", "not_active");
    $('#prev').attr("class", "active");
}
else {
    $('#prev').attr("class", "active")
    $('#next').attr("class", "active")
};

I've made a jsFiddle and included the complete code below:



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

//////////////////////////// INITAL SET UP /////////////////////////////////////////////

//Get size of images, how many there are, then determin the size of the image reel.
var divWidth = $(".window").width();
var divSum = $(".slide").size();
var divReelWidth = divWidth * divSum;

//Adjust the image reel to its new size
$(".image_reel").css({ 'width': divReelWidth });

//set the initial not active state
$('#prev').attr("class", "not_active");

//////////////////////////// SLIDER /////////////////////////////////////////////

//Paging + Slider Function
rotate = function () {
var triggerID = $slideNumber - 1; //Get number of times to slide
var image_reelPosition = triggerID * divWidth; //Determines the distance the image reel needs to slide
//sets the active on the next and prev
if ($slideNumber == 1) {
$('#prev').attr("class", "not_active")
$('#next').attr("class", "active")
}
e

1) Separation of Concerns

Start by refactorring your code in to more granular functions.
You can read more about SoF at http://en.wikipedia.org/wiki/Separation_of_concerns

Update:
E.g. Instead of having your reel resizing code inline, put it in it's own function, like this:

function setImageReelWidth () {
    //Get size of images, how many there are, then determin the size of the image reel.
    var divWidth = $(".window").width();
    var divSum = $(".slide").size();
    var divReelWidth = divWidth * divSum;

    //Adjust the image reel to its new size
    $(".image_reel").css({ 'width': divReelWidth });                            
}

This achieves 2 things:

a. First, it groups a block of code that is logically cohesive, removing it from the main code which results in a much cleaner code habitat.
b. It effectively gives a label to the code block via the function name that is descriptive of what it does, and therefore makes understanding of the code much simpler.

Later, you can also encapsulate the whole thing in it's own "class" (function) and you can move it into it's own js file.

2) The jQuery "on" function

Use the "on" function to attach your click events, rather than the "click" function.
http://api.jquery.com/on/
This has the added advantage of also binding it to future elements matching your selector, even though they do not exist yet.

3) The ready function

// I like the more succinct:
$(handler)
// Instead of:
$(document).ready(handler)

But you might like the more obvious syntax.

Those are just a few things to start with.

-- Update 1 --

I think you should keep your original code block in your question, so that future readers can see where it started and how it systematically improved.


I would like to know more about the create a new function, outside of
the jQuery ready block as i cant get this working or quite understand
how i can get it to work sorry

I am not familiar with jsfiddle.net, but it looks cool and helpful, but might also be a bit confusing if you don't know what is going on. I am not sure I do :), but I think that script editor window results in a .js file that is automatically referenced by the html file.

So here is an example of a function defined outside of the ready block, but referenced from within.

function testFunction () {
    alert ('it works');
}
$(document).ready(function () {
    testFunction();

    // ... other code
});

This should pop up an alert box that says, "it works" when the page is loaded.
You can try it for yourself.
Then, once you got that working, you can refactor other logically cohesive blocks of code into their own functions. Later you can wrap them all up into their own javascript 'class'. But we'll get to that.


also is there a better way to write:

if ($slideNumber == 1) {
    $('#prev').attr("class", "not_active")
    $('#next').attr("class", "active")
}
else if ($slideNumber == divSum) {
    $('#next').attr("class", "not_active");
    $('#prev').attr("class", "active");
}
else {
    $('#prev').attr("class", "active")
    $('#next').attr("class", "active")
};

One option is to write it like this:

$('#prev').attr("class", "not_active")
$('#next').attr("class", "not_active")
if ($slideNumber > 1)
    $('#prev').attr("class", "active")
if ($slideNumber < divSum )
    $('#next').attr("class", "active")

StackExchange Code Review Q#8634, answer score: 4