5
\$\begingroup\$

I built a very basic image slider, but I'm still having trouble refactoring.

I caught myself reusing same code for clicking the previous/next buttons, selecting a slider dot, and the automated sliding. The difference between these actions is how the index of images changes. I'm not sure how to break out that piece of code "efficiently".

Here's the sample code: JSFiddle

As an example of duplicate code that I'm not sure how to refactor efficiently:

If I want to select a slider dot:

$("#dots li").click(function() {
    var select = $("#dots li").index(this);
    dotItems.removeClass('active');
    listItems.eq(i).fadeOut(transition_speed);
    i = select;
    dotItems.eq(i).addClass('active');
    listItems.eq(i).fadeIn(transition_speed);
  });

And if I want to select the previous button:

$("#prev").click(function() {
    dotItems.removeClass('active');
    listItems.eq(i).fadeOut(transition_speed);
    i = i >= 0 ? i - 1 : listLen - 1;
    dotItems.eq(i).addClass('active');
    listItems.eq(i).fadeIn(transition_speed);
  });

Hope somebody can take a look at it and at least point me in the right direction.

My background: Worked in Finance a few years, currently self-studying web development ( < 1 year). Still new to programming, and it may be that I'm still learning to think like a programmer.

\$\endgroup\$
1
\$\begingroup\$

Here's my take on it (jsfiddle)

$(document).ready(function () {

var change_img_time = 5000,
    transition_speed = 500,
    autoplay = true;

var listItems = $("#slider").children('li'),
    dotItems = $('#dots').children('li'),
    listLen = listItems.length,
    current;

   // listItems.not(':first').hide(); -- handle this with css instead to avoid seeing a flash of all the images on load

You can do all your navigating with one function, just by changing what you pass to it:

function moveTo(newIndex) {

    var i = newIndex;

    if (newIndex == 'prev') {
        i = (current > 0) ? (current - 1) : (listLen - 1);
    }

    if (newIndex == 'next') {
        i = (current < listLen - 1) ? (current + 1) : 0;
    }
    // if it's not 'prev' or 'next', we'll assume it's a number

    dotItems.removeClass('active')
        .eq(i).addClass('active');

    listItems.fadeOut(transition_speed)
        .eq(i).fadeIn(transition_speed);

    current = i;
};

Then your event handlers could be:

$("#dots li").click(function () {
    var i = $('#dots li').index(this);
    moveTo(i);
});

$("#prev").click(function () {
    moveTo('prev');
});

$("#next").click(function () {
    moveTo('next');
});

(You could even get clever with this if you wanted and do this instead:)

$('button').click(function() {
    var action = $(this).id();
    moveTo(action);
});

*/

At this point you could just throw your 'setInterval' on it and call it a day, but why not wrap it up a little? (Remember the 'autoplay' variable that showed up earlier?) This way you have more control over when/if the slideshow starts running.

startSlideshow = function() {
    if (autoplay == true) {
        setInterval(function () {
            i = current;
            moveTo('next');
        }, change_img_time);
    }
};

// Optional: set a timeout so the images have a moment to load:

    // setTimeout(startSlideshow, 500);

// or just go ahead without a delay:

    startSlideshow();


});

These are basic optimizations, but ideally you should take this a little further and wrap it up in a more modular/portable container, to avoid possible conflicts with other page scripting. This could be as simple as wrapping the whole thing in a function:

Slideshow = function() {
  ... code goes here ...
}

after which you'd initialize it like Slideshow.startSlideshow();

But that's a very general explanation. I definitely recommend reading a bit about JS plugin/module patterns, and how to write them to keep your code neat and self-contained.

\$\endgroup\$
  • \$\begingroup\$ awesome code. hope to get up to this level someday. quick follow-up questions regarding startSlideShow function: is there a reason you included i = current? Why didn't use var when you declared function? And as @schism mentioned, would you also agree that setTimeOut would be more appropriate than setInterval in this case? also i found this jQuery link on module patterns. Are there any other resources you know that can get me up to speed? \$\endgroup\$ – Jamie B Jul 4 '14 at 14:15
3
\$\begingroup\$
  1. Put it in a separate function, and call it whenever you want to change it.
  2. I'm not a fan of using an application-scope variable to store the currently selected frame. You could use something like listItems.index(listItems.filter(":visible")) to get the index of the currently selected frame. However, if you want to keep using your way, then please name it something better.

I made a few changes. Of note:

  1. I set display: none on #slider li instead of using listItems.not(':first').hide(). This is because when I loaded it the first time, all the images were briefly shown. This will prevent them from being rendered initially.
  2. I changed the interval into a timeout that's refreshed upon changing frames. This way, if I click on a dot 4.5 seconds after the frame changes, the slider won't change my frame after just 0.5 seconds.

Here's a fiddle.


I made a few more changes in an updated version. Specifically:

  1. By convention, constants are named in uppercase.
  2. By convention, jQuery variables are prefixed with $.
  3. I got rid of the selectedFrame variable and replaced it with a getSelectedIndex() function.
  4. I made the dots dynamically added.

Here's a fiddle.

\$\endgroup\$
1
\$\begingroup\$

You are basically doing 3 things in both functions:

  1. Make every dot inactive
  2. Determine which dot is now active
  3. Activate the determined dot

You could easily make 1) a part of 3) and create a function that activates a dot:

function activateDot( dotIndex  ){
   dotItems.removeClass('active');
   dotItems.eq( dotIndex ).addClass('active');
   listItems.eq( dotIndex ).fadeIn(transition_speed);   
}

Then your 2 handlers could look like

$("#dots li").click(function() {
    var selectedDot = $("#dots li").index(this);
    activateDot( selectedDot );
  });

$("#prev").click(function() {
    var currentDot = $("#dots li .active").index(),
    selectedDot = currentDot ? currentDot - 1 : listLen - 1;
    activateDot( selectedDot );
  });

I ignored the fadeOut part since i is not known in $("#prev").click. Unless i is a global variables in which case you have a lot of rethinking ahead of you, no global variables please!!

\$\endgroup\$

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.