patternjavascriptMinor
DRYing out a basic image slider
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:
And if I want to select the previous button:
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.
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: noneon#slider liinstead of usinglistItems.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
intervalinto atimeoutthat'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
selectedFramevariable and replaced it with agetSelectedIndex()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.