jQuery slideshow with connected links and the ability to pause/resume when hovered

Question

Update: Here is Take 2.

---

I want a slideshow with three primary pieces of functionality:

1. Has text links that act as bullet-points and can jump directly to a slide when clicked.
2. Pauses when a slide or link is moused over.
3. Resumes resumes playing from the currently active slide if the user clicks a link.

This is what I came up with. Thanks for your feedback.

$(function() {
  var slideCount = $("#carousel-slides > img").length
  var slideToActivate = 1
  var viewingLength = 2000
  var loop

  automateTheCarousel()


  /* === events handlers === */
  $("#carousel-nav").on("click", "a", function() {
    slideToActivate = $(this).data("linked-image")
    changeCarouselSlide()
  })

  $("#carousel-slides, #carousel-nav").hover(stopTheCarousel, automateTheCarousel)


  /* === functions === */
  function automateTheCarousel() {
    loop = setInterval(goToNextCarouselSlide, viewingLength)
  }

  function stopTheCarousel() {
    clearInterval(loop)
  }

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

  function changeCarouselSlide() {
    // Deactivate everything.
    $("#carousel-slides img").hide()
    $("#carousel-nav a").css({
      "font-weight": "normal"
    })

    // Activate
    $("#" + slideToActivate).show()
    $("a[data-linked-image=" + slideToActivate + "]").css({
      "font-weight": "bold"
    })
  }

});

#carousel-slides {
  float: left;
  width: 50%;
}
#carousel-slides img {
  height: 200px;
  width: 200px;
  display: block;
  margin: 0 auto;
}
#carousel-nav {
  float: right;
  width: 50%;
  text-align: left;
}

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div id="carousel-slides">
  <img id="1" src="https://i.sstatic.net/aKmK0.png">
  <img id="2" style="display: none" src="https://i.sstatic.net/4V0IS.jpg">
  <img id="3" style="display: none" src="https://i.sstatic.net/QtNO8.jpg">
</div>

<div id="carousel-nav">
  These are the tests!!!
  <br>
  <a href="#" data-linked-image="1" style="font-weight: bold" }>Test 1</a>
  <br>
  <a href="#" data-linked-image="2">Test 2</a>
  <br>
  <a href="#" data-linked-image="3">Test 3</a>
  <br>And here is some more text.
</div>

One small note: the inline styling is there to visually set the first slide as "active" when the page loads. I feel like there's a better way, though.

Answer 1

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.