HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

DRYing out a basic image slider

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
imagedryingslideroutbasic

Problem

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.

Solution


  • Put it in a separate function, and call it whenever you want to change it.



  • 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:

  • 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.



  • 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:

  • By convention, constants are named in uppercase.



  • By convention, jQuery variables are prefixed with $.



  • I got rid of the selectedFrame variable and replaced it with a getSelectedIndex() function.



  • I made the dots dynamically added.



Here's a fiddle.

Context

StackExchange Code Review Q#55993, answer score: 3

Revisions (0)

No revisions yet.