1
\$\begingroup\$

I have a function that changes the image on a webpage every three seconds. I would like feedback on efficiency, good code practices, and anything else that can improve the quality of this code. It works, I just want any insight if it can work any better. Any and all feedback is welcomed and considered.

Slideshow Script

<!-- Script for Slide Show: Change picture every three seconds -->
<script type="text/javascript">
    var index = 0;
    change();

    function change() {

        //Collect all images with class 'slides'
        var x = document.getElementsByClassName('slides');

        //Set all the images display to 'none' (invisible)
        for(var i = 0; i < x.length; i++) { 
            x[i].style.display = "none"; 
        }

        //Increment index variable
        index++;

        //Set index to 1 if it's greater than the amount of images
        if(index > x.length) { 
            index = 1; 
        }

        //set image display to 'block' (visible)
        x[index - 1].style.display = "block";

        //set loop to change image every 3000 milliseconds (3 seconds)
        setTimeout(change, 3000);
    }
</script>

HTML Images accessed

<!-- Slide Show -->
<section>
    <img class="slides" src="external/ph1.jpg" style="width:100%">
    <img class="slides" src="external/ph2.jpg" style="width:100%">
    <img class="slides" src="external/ph3.jpg" style="width:100%">
</section>
\$\endgroup\$
3
\$\begingroup\$
  • The script tag default type is text/javascript and thus is not needed.

  • Keep your script out of the global scope. This can become a problem when more JS is added to the page and you get name conflicts. The easiest way to encapsulate your code is to wrap it in a function. In this case it would be best to wait for the page to load before starting image cycle. You can use either DOMContentLoaded or load events but if you use the first be sure that the CSS has loaded.

  • Drop the comments they add nothing that is not self evident in the code itself, and are but noise. If you find that the code does not contain the information needed to understand it, first can you rename to make it clear, if not then comments are the last resort.

  • Define a CSS class for hidden images rather than setting the style properties directly. (see example)

  • Use the CSS class to define style properties. Do not set style properties in the HTML document. Its just easier to manage.

  • Use querySelectorAll it is more flexible in the long run.

  • Use for of loop rather than for(;;) loops as it cleaner and requires less code.

  • You don't need to query for the images every 3 seconds. Do it once at the start.

  • Use const for variables that do not change.

  • The variable name x is a bad name. slideImages of slides would be more fitting. Note that I use the plural signifying that it is array, or array like.

  • Start with the images hidden, then ever 3 seconds all you need to do is hide one and show one.

  • Use the remainder operator % to cycle a value (see example)

Example

<style>
    .slides { width: 100%; }
    .slides-hidden { display : none; }
</style>

<script>
    addEventListener("load",() => { // "load" is safe but "DOMContentLoaded" starts earlier
        var index = 0;
        const slides = document.querySelectorAll(".slides");
        const classHide = "slides-hidden", count = slides.length;
        nextSlide();
        function nextSlide() {
            slides[(index ++) % count].classList.add(classHide);
            slides[index % count].classList.remove(classHide);
            setTimeout(nextSlide, 3000);
        }
    });
</script>

<section>
    <img class="slides slides-hidden" src="external/ph1.jpg">
    <img class="slides slides-hidden" src="external/ph2.jpg">
    <img class="slides slides-hidden" src="external/ph3.jpg">
</section>
\$\endgroup\$
0
\$\begingroup\$

I made few improvements, the current code you have looks clean to me. But it's better to have control over something we created. So when I looked at the code, I got these questions:

  1. What if I want to go to specific slide based on some logic.
  2. Why can't we make it reusable
  3. Why can't we make it context bound so that it has it's specific context to store state.
  4. What if I want to destroy any point of time.

var SlideShow = (function () {
  function SlideShow (config) {
    if (!config) {
      config = {};
    }
    this.slideSelector = config.slideSelector;
    this.refreshInterval = config.refreshInterval || 3000;
    this.currentVisibleSlide = null;
    this.initialize();
  }
  var prototype = {
    constructor: SlideShow
  };
  prototype.initialize = function () {
    this.refresh();
    this.nextSlide();
    this.slideShowTimer = window.setInterval(this.nextSlide.bind(this), this.refreshInterval);
  }
  prototype.destroy = function () {
    if (this.slideShowTimer) {
      window.clearInterval(this.slideShowTimer);
    }
  }
  prototype.displaySlide = function (slide, show) {
    slide && (slide.style.display = show ? 'block' : 'none');
  }
  prototype.gotoSlide = function (index) {
    var slideToShow = this.slideElements[index];
    if (slideToShow) {
      if (this.currentVisibleSlide) {
        this.displaySlide(this.currentVisibleSlide, false);
      }
      this.displaySlide(slideToShow, true);
      this.currentVisibleSlide = slideToShow;
    }
  }
  prototype.nextSlide = function () {
    var currentVisibleSlide = this.currentVisibleSlide
    var nextSlideIndex;
    if (!currentVisibleSlide) {
      nextSlideIndex = 0;
    } else {
      currentVisibleSlide = this.slideElements.indexOf(currentVisibleSlide);
      nextSlideIndex = currentVisibleSlide + 1;
      if (nextSlideIndex > this.slideElements.length - 1) {
        nextSlideIndex = 0;
      }
    }
    console.log('Showing index: ', nextSlideIndex);
    this.gotoSlide(nextSlideIndex);
  }
  prototype.refresh = function () {
    var slideElements = document.querySelectorAll(this.slideSelector);
    this.slideElements = Array.prototype.slice.call(slideElements, 0);
  }
  SlideShow.prototype = prototype;
  return SlideShow;
})();


var slideShow = new SlideShow({
  slideSelector: '.slides'
})
.slides {
  display: none
}
<section>
    <div class="slides" style="width:100%">1</div>
    <div class="slides" style="width:100%">2</div>
    <div class="slides" style="width:100%">3</div>
</section>

Edit:

My rationale behind converting it to a class is, In our application we had an exact same functionality written by a beginner JS programmer in a same way as OP has posted. Other developers made amendments to it. The code has grown complex and started giving unexpected bugs. It was harder to maintain and reuse in other parts of the application. So my intention behind rewriting it was to show an alternate way in a way that i thought will be easier to maintain. And it does not have comments because, comments lose meaning as code changes, but variable names do not. Thats why variables in above code has some meaning behind their names.

And another intention behind making it class is, it is easy to extend. UX requirements changes very rapidly. Tomorrow OP might have to show all the slides and switch them using timer. UX team might ask to click and show the slide instead of showing in automated way.

Thank you for the excellent inputs in comments section.

\$\endgroup\$
  • 3
    \$\begingroup\$ Just posting an alternative implementation isn't the goal of this site. You should explain why you consider your solution better. To be honest I find your solution over-engineered for the given task. \$\endgroup\$ – RoToRa Mar 22 at 8:22
  • \$\begingroup\$ @RoToRa Thanks for clarifying. I thought my explanation was enough to showcase what problems it is solving but looks like not. Also can you tell me more about why do you think its over engineered? I am curious. \$\endgroup\$ – brightDot Mar 22 at 16:45
  • \$\begingroup\$ I agree that it is over cooked. Code complexity is source of bugs, and feature creep is a common reason for undue complexity, You have added features not required, to accommodate these features you have had to add a lot more code (5 times as much as is needed). In addition the expanded features have been added such to expose an unsafe interface that if not used correctly will throw or produce unexpected behavior. Keep it simple is an important part of good code design. \$\endgroup\$ – Blindman67 Mar 22 at 19:44
  • \$\begingroup\$ I really enjoy your code, which is why I wish I didn't have to say that the scope creep is obscuring key lessons for the OP. Not only the amount of extra code the OP must digest but also this ensuing commentary. \$\endgroup\$ – radarbob Mar 23 at 3:59
  • \$\begingroup\$ I winced when I saw var prototype = {...}. I can just see this happening prototype.prototype.anotherFunction = .... Even this: prototype = ... would cause anyone to pause in uncertainty. The potential confusion is not worth it. \$\endgroup\$ – radarbob Mar 23 at 4:28

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.