Objects

Your code is fairly spread out and not very portable. All your variables are living in the top scope (well, not the global scope, but in your first $ call).

It would be more beneficial to pack this code into an object/class which keeps the state contained in the object itself. You could perhaps create something like this:

function ImageCarousel(slides, links) {
 this.slides = slides;
 this.links = links;
 // Here, you can setup the state of the carousel.
 this.slideToActivate = 1;
 this.viewingLength = 2000;
 this.loop = null;
}

With this design, nothing is leaking out into outer scopes and you can keep all the logic and state together.

Expensiveness

$ is an expensive function. It traverses the whole DOM every time you call it (or most of it), and this is certainly no speedy task.

$("#carousel-nav")

and

$("#carousel-slides...")

pop up more times than they should. Are the returns of these functions changing at any point in the program? No. Therefore, the results should be stored in variables

var navLinks = $("#carousel-nav");
var slides = $("#carousel-slides...");

Then, you can just access these variables rather than calling the function every time.

Simplify operation

slideToActivate++
if (slideToActivate > slideCount) {
 slideToActivate = 1
}
changeCarouselSlide()

can be simplified to

slideToActivate = (slideToActive + 1) % slideCount;
changeCarouselSlide()

The remainder operator (%) wraps numbers back around to 0 if they pass the right hand value.

Necessarianess

// Deactivate everything.

Is this really necessary? You have (or you should have) all the information you need to turn off the exact slides/links that are activated. For setups where there are a lot of slides and links, your current solution can get slow.

Well, we know the current slide number and we have an array of slides and links...

slides[slideToActivate].hide();
navLinks[slideToActivate].css(...);

##ID's

Your current system of ID's is a little strange.

<img id="1"...
<img id="2"...
<img id="3"...

These ID's aren't descriptive at all of what the slides are. Here are some possible things you can do:

• Change the ids to something like "imagecarousel-slide-1".
• Use a data property like imagecarousel-slide="1".
• Assume the slides are in the order in which they will appear.

The last method is certainly the easiest to implement. If you do choose one of the two former methods though, make sure that when you stick them in the array I mentioned above (navLinks or slides) you order them based on the property you decided.

Objects

Your code is fairly spread out and not very portable. All your variables are living in the top scope (well, not the global scope, but in your first $ call).

It would be more beneficial to pack this code into an object/class which keeps the state contained in the object itself. You could perhaps create something like this:

function ImageCarousel(slides, links) {
 this.slides = slides;
 this.links = links;
 // Here, you can setup the state of the carousel.
 this.slideToActivate = 1;
 this.viewingLength = 2000;
 this.loop = null;
}

With this design, nothing is leaking out into outer scopes and you can keep all the logic and state together.

Expensiveness

$ is an expensive function. It traverses the whole DOM every time you call it (or most of it), and this is certainly no speedy task.

$("#carousel-nav")

and

$("#carousel-slides...")

pop up more times than they should. Are the returns of these functions changing at any point in the program? No. Therefore, the results should be stored in variables

var navLinks = $("#carousel-nav");
var slides = $("#carousel-slides...");

Then, you can just access these variables rather than calling the function every time.

Simplify operation

slideToActivate++
if (slideToActivate > slideCount) {
 slideToActivate = 1
}
changeCarouselSlide()

can be simplified to

slideToActivate = (slideToActive + 1) % slideCount;
changeCarouselSlide()

The remainder operator (%) wraps numbers back around to 0 if they pass the right hand value.

Necessarianess

// Deactivate everything.

Is this really necessary? You have (or you should have) all the information you need to turn off the exact slides/links that are activated. For setups where there are a lot of slides and links, your current solution can get slow.

Well, we know the current slide number and we have an array of slides and links...

slides[slideToActivate].hide();
navLinks[slideToActivate].css(...);

ID's

Your current system of ID's is a little strange.

<img id="1"...
<img id="2"...
<img id="3"...

These ID's aren't descriptive at all of what the slides are. Here are some possible things you can do:

• Change the ids to something like "imagecarousel-slide-1".
• Use a data property like imagecarousel-slide="1".
• Assume the slides are in the order in which they will appear.

The last method is certainly the easiest to implement. If you do choose one of the two former methods though, make sure that when you stick them in the array I mentioned above (navLinks or slides) you order them based on the property you decided.

Objects

Your code is fairly spread out and not very portable. All your variables are living in the top scope (well, not the global scope, but in your first $ call).

It would be more beneficial to pack this code into an object/class which keeps the state contained in the object itself. You could perhaps create something like this:

function ImageCarousel(slides, links) {
 this.slides = slides;
 this.links = links;
 // Here, you can setup the state of the carousel.
 this.slideToActivate = 1;
 this.viewingLength = 2000;
 this.loop = null;
}

With this design, nothing is leaking out into outer scopes and you can keep all the logic and state together.

Expensiveness

$ is an expensive function. It traverses the whole DOM every time you call it (or most of it), and this is certainly no speedy task.

$("#carousel-nav")

and

$("#carousel-slides...")

pop up more times than they should. Are the returns of these functions changing at any point in the program? No. Therefore, the results should be stored in variables

var navLinks = $("#carousel-nav");
var slides = $("#carousel-slides...");

Then, you can just access these variables rather than calling the function every time.

Simplify operation

slideToActivate++
if (slideToActivate > slideCount) {
 slideToActivate = 1
}
changeCarouselSlide()

can be simplified to

slideToActivate = (slideToActive + 1) % slideCount;
changeCarouselSlide()

The modulus operator (%) wraps numbers back around to 0 if they pass the right hand value.

Necessarianess

// Deactivate everything.

Is this really necessary? You have (or you should have) all the information you need to turn off the exact slides/links that are activated. For setups where there are a lot of slides and links, your current solution can get slow.

Well, we know the current slide number and we have an array of slides and links...

slides[slideToActivate].hide();
navLinks[slideToActivate].css(...);

##ID's

Your current system of ID's is a little strange.

<img id="1"...
<img id="2"...
<img id="3"...

These ID's aren't descriptive at all of what the slides are. Here are some possible things you can do:

• Change the ids to something like "imagecarousel-slide-1".
• Use a data property like imagecarousel-slide="1".
• Assume the slides are in the order in which they will appear.

The last method is certainly the easiest to implement. If you do choose one of the two former methods though, make sure that when you stick them in the array I mentioned above (navLinks or slides) you order them based on the property you decided.

Objects

Your code is fairly spread out and not very portable. All your variables are living in the top scope (well, not the global scope, but in your first $ call).

It would be more beneficial to pack this code into an object/class which keeps the state contained in the object itself. You could perhaps create something like this:

function ImageCarousel(slides, links) {
 this.slides = slides;
 this.links = links;
 // Here, you can setup the state of the carousel.
 this.slideToActivate = 1;
 this.viewingLength = 2000;
 this.loop = null;
}

With this design, nothing is leaking out into outer scopes and you can keep all the logic and state together.

Expensiveness

$ is an expensive function. It traverses the whole DOM every time you call it (or most of it), and this is certainly no speedy task.

$("#carousel-nav")

and

$("#carousel-slides...")

pop up more times than they should. Are the returns of these functions changing at any point in the program? No. Therefore, the results should be stored in variables

var navLinks = $("#carousel-nav");
var slides = $("#carousel-slides...");

Then, you can just access these variables rather than calling the function every time.

Simplify operation

slideToActivate++
if (slideToActivate > slideCount) {
 slideToActivate = 1
}
changeCarouselSlide()

can be simplified to

slideToActivate = (slideToActive + 1) % slideCount;
changeCarouselSlide()

The remainder operator (%) wraps numbers back around to 0 if they pass the right hand value.

Necessarianess

// Deactivate everything.

Is this really necessary? You have (or you should have) all the information you need to turn off the exact slides/links that are activated. For setups where there are a lot of slides and links, your current solution can get slow.

Well, we know the current slide number and we have an array of slides and links...

slides[slideToActivate].hide();
navLinks[slideToActivate].css(...);

##ID's

Your current system of ID's is a little strange.

<img id="1"...
<img id="2"...
<img id="3"...

These ID's aren't descriptive at all of what the slides are. Here are some possible things you can do:

• Change the ids to something like "imagecarousel-slide-1".
• Use a data property like imagecarousel-slide="1".
• Assume the slides are in the order in which they will appear.

The last method is certainly the easiest to implement. If you do choose one of the two former methods though, make sure that when you stick them in the array I mentioned above (navLinks or slides) you order them based on the property you decided.