7
\$\begingroup\$

I've made a very simple slideshow that works by appending and prepending images (on click and every 3 seconds). This is my first time trying to use classes or objects so any help would be great. I feel this could be a little verbose so I'd love any sort of feedback.

The user can click through the slideshow, but it also works on a timer of every 3 seconds.

 // Scroll through images on the homepage
 function IndexSlideshow() {
 // declare index page html elements
 var contentSlideshow = $('.content-slideshow'),
 slide = $('.slide'),
 body = $('body'),
 self = this;
 // append the first slide, fade in the newly bumped slide 
 this.work = function() {
 $('.slide:first').fadeOut(200, function() {
 $(this).appendTo(contentSlideshow);
 $('.slide:first').hide().fadeIn(200);
 });
 }
 // show previous slide on click
 this.showPrev = function() {
 slide.click(function() {
 $(this).prependTo(contentSlideshow);
 });
 }
 // show next slide on click
 this.showNext = function() {
 slide.click(function() {
 self.work();
 });
 }
 // create an auto slide (slides every 3 seconds)
 this.auto = function() {
 var run;
 if (body.hasClass('home')) {
 run = setInterval(function() {
 self.work();
 }, 3000);
 // reset interval if they clicked an image
 slide.click(function() {
 clearInterval(run);
 run = setInterval(function() {
 self.work();
 }, 3000);
 });
 };
 }
 }
 var i = new IndexSlideshow();
 i.showNext();
 i.auto();
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Aug 27, 2014 at 14:22
\$\endgroup\$
2
  • 2
    \$\begingroup\$ Can you should the HTML you want this to work on? \$\endgroup\$ Commented Aug 28, 2014 at 13:39
  • 1
    \$\begingroup\$ When does showPrev() come into play here? The HTML would definitely be helpful here so we can see how you're using the functions. Your code isn't bad at all, but I do see some opportunity for improvement. I just need a little more info. \$\endgroup\$ Commented Nov 17, 2014 at 21:51

1 Answer 1

2
\$\begingroup\$

I like this code;

  • There are perhaps a few too many newlines, especially here:

     });
     };
     }
    }
    
  • work and auto are unfortunate function names, you could do better
  • slide = $('.slide'), <- I would have called that slides since you are almost bound to select more than 1 slide with this selector
  • The repeated magical constants 3000 and 200 should be identified, named and centralized
answered Nov 24, 2014 at 19:48
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.