Why is this JavaScript Loop not Working?

Question

EDIT: I'm having a new issue with this. Next() was changed to the following:

  var nextClicked = 0;
  function next()
  {
    nextClicked = 1; 
    cycler(); 
  }

I have also tried:

  var nextClicked = 0;
  function next()
  {
    nextClicked++; 
    cycler(); 
  }

In both cases, when the Next function fires, the change in the nextClicked variable value never occurs. I have watched the line being read in the Chrome Inspector, but the variable value never changes. I am at a loss. And I have not found any references to this in searching (although someone with more familiarity with past questions may have more luck). Can anyone explain this bizarre behavior? Thank you!

EDIT: Removing the parenthesis, as recommended below, solved the problem of the image cycling failing to execute, thank you very much.

This is a JavaScript image slide show. Each image displays for 5 seconds, then the next is displayed. When the final image is reached, the cycle restarts. Everything was working perfectly when I had this code written out in sequence:

  <script type="text/javascript>
  var clicked = 0;  // repeatedly clicking slide show button disrupts image cycling, trap this error.  
  function cycle()
  {      
    clicked += 1; 
    if (clicked > 1) return; 
    cycler(); 
  } 

  function cycler()
  {        
    setTimeout(image1, 0000);  
    setTimeout(image2, 5000); 
    setTimeout(image3, 10000);
    setTimeout(image4, 15000);  
    setTimeout(image5, 20000); 
    setTimeout(image6, 25000);
    setTimeout(image7, 30000);     
    setTimeout(image8, 35000); 
    setTimeout(image9, 40000);
  }

  var navImages = '<br><br><img style="float:left;" src="back.png" alt="Previous Image" width="160" height="80" /><img style="float:right;" src="next.png" alt="Next Image" width="160" height="80" />';   
  function image1()
  { 
    document.getElementById("cycle").innerHTML = '<img src="floating_city.jpg" alt="Floating City" width="800" height="532" /><br><div>Floating City - Created in Voyager</div>' + navImages;        
    document.getElementById("cycle").scrollIntoView();
  }   
  function image2() {document.getElementById("cycle").innerHTML = '<img src="frozen_lake.jpg" alt="Frozen Lake" width="800" height="532" /><br><div>Frozen Lake with Alien Antenna - Created in Voyager</div>' + navImages};   
  function image3() {document.getElementById("cycle").innerHTML = '<img src="paradise_peak.jpg" alt="Paradies Peak" width="800" height="532" /><br><div>Paradise Peak - Created in Voyager</div>' + navImages};   
  function image4() {document.getElementById("cycle").innerHTML = '<img src="northern_moon.jpg" alt="Northern Moon" width="800" height="532" /><br><div>Northern Moon - Created in MojoWorld</div>' + navImages};     
  function image5() {document.getElementById("cycle").innerHTML = '<img src="woman_on_alien_beach.jpg" alt="Woman on Alien Beach" width="800" height="532" /><br><div>Woman on Alien Beach - Landscape Created in Voyager - Figure Created in Poser</div>' + navImages}; 
  function image6() {document.getElementById("cycle").innerHTML = '<img src="mount_vanilla.jpg" alt="mount_vanilla" width="800" height="532" /><div>Mount Vanilla - Created in Voyager</div>' + navImages};  
  function image7() {document.getElementById("cycle").innerHTML = '<img src="blue_orbs.jpg" alt="Blue Orbs" width="800" height="532" /><div>Blue Orbs - Created in Bryce</div>' + navImages}; 
  function image8() {document.getElementById("cycle").innerHTML = '<img src="ufo_city.jpg" alt="UFO City" width="800" height="532" /><div>UFO Invasion - Created in Bryce - Pyrotechnics Created in Particle Illusion</div>' + navImages};  
  function image9() {cycler()};  // Create infinite loop (without crashing browser). 
  </script>

Then, I tried to add code for the Back and Next buttons. This basically required that code be inserted after every setTimeout line, so that the image# function could be moved to the next (or previous) image# function, the timeout reset, and the cycler() function exited. All that code seemed redundant, so I placed the code in a loop.

  <script type="text/javascript">
  <!--

  var clicked = 0;    
  var imageNumber = 1; 
  var mSeconds = 0; 
  var nextClicked = 0;
  var prevClicked = 0;   
  var callFunction = '';   

  function cycle()
  {      
    clicked += 1;  // repeatedly clicking slide show button disrupts image cycling, trap this error.
    if (clicked > 1) return; 
    cycler(); 
  }

  function cycler()
  {        
    for (i = 1; i < 10; i++)
    {  
      if (nextClicked == 1)
      {
        imageNumber++; 
        mSeconds = 0; 
        window['image' + imageNumber]();      
      }

      if (prevClicked == 1)
      {
        imageNumber--; 
        mSeconds = 0; 
        window['image' + imageNumber]();      
      }

    callFunction = 'image' + imageNumber;  
    setTimeout(window[callFunction](), mSeconds);  // call image# function, set time
    imageNumber += 1; 
    mSeconds += 5000; 
    if (imageNumber == 9) {imageNumber = 1}; 
    if (mSeconds == 45000) {mSeconds = 0}; 
    }
  }  

  var navImages = '<br><br><img style="float:left;" src="back.png" alt="Previous Image" width="160" height="80" onClick="prev()" /><img style="float:right;" src="next.png" alt="Next Image" width="160" height="80" onClick="next()" />';   
  function image1()
  { 
    document.getElementById("cycle").innerHTML = '<img src="floating_city.jpg" alt="Floating City" width="800" height="532" /><br><div>Floating City - Created in Voyager</div>' + navImages;        
    document.getElementById("cycle").scrollIntoView();
  }   
  function image2() {document.getElementById("cycle").innerHTML = '<img src="frozen_lake.jpg" alt="Frozen Lake" width="800" height="532" /><br><div>Frozen Lake with Alien Antenna - Created in Voyager</div>' + navImages};   
  function image3() {document.getElementById("cycle").innerHTML = '<img src="paradise_peak.jpg" alt="Paradies Peak" width="800" height="532" /><br><div>Paradise Peak - Created in Voyager</div>' + navImages};   
  function image4() {document.getElementById("cycle").innerHTML = '<img src="northern_moon.jpg" alt="Northern Moon" width="800" height="532" /><br><div>Northern Moon - Created in MojoWorld</div>' + navImages};     
  function image5() {document.getElementById("cycle").innerHTML = '<img src="woman_on_alien_beach.jpg" alt="Woman on Alien Beach" width="800" height="532" /><br><div>Woman on Alien Beach - Landscape Created in Voyager - Figure Created in Poser</div>' + navImages}; 
  function image6() {document.getElementById("cycle").innerHTML = '<img src="mount_vanilla.jpg" alt="mount_vanilla" width="800" height="532" /><div>Mount Vanilla - Created in Voyager</div>' + navImages};  
  function image7() {document.getElementById("cycle").innerHTML = '<img src="blue_orbs.jpg" alt="Blue Orbs" width="800" height="532" /><div>Blue Orbs - Created in Bryce</div>' + navImages}; 
  function image8() {document.getElementById("cycle").innerHTML = '<img src="ufo_city.jpg" alt="UFO City" width="800" height="532" /><div>UFO Invasion - Created in Bryce - Pyrotechnics Created in Particle Illusion</div>' + navImages};  
  function image9() {cycler()};  // Create infinite loop (without crashing browser). 

  function next() {nextClicked = 1}; 
  function prev() {prevClicked = 1}; 

  --> 
</script>

And... Now it doesn't work. The first image shows up, and that's it. Back doesn't work, and next doesn't work. When I set a breakpoint and go line by line in Chrome, the text for the images shows up, as if everything is normal. But at runtime, it just gets stuck on the first image.

I'm new to JavaScript, suggestions are appreciated.

Possible duplicate of [Why is the method executed immediately when I use setTimeout?](http://stackoverflow.com/questions/7137401/why-is-the-method-executed-immediately-when-i-use-settimeout) — JJJ, Apr 15 '17 at 05:50
Also, not related to the problem, but making a separate function for each image isn't the cleanest way to implement this. Put the images in an array and pull them from there with just one function. — JJJ, Apr 15 '17 at 05:52
You have `clicked += 1` but never seem to reset it, so any clicks after the first are ignored. — RobG, Apr 15 '17 at 06:03
That is correct behavior, RobG. One extra click is all it takes to throw off the image cycling, so the function exits after an additional click. — TonyLuigiC, Apr 15 '17 at 06:05
You haven't shown the buttons but the `next()` and `prev()` functions don't do anything apart from setting a variable. — JJJ, Apr 15 '17 at 06:12
The buttons are there, JJJ. Please see the second "var navImages = " They set a variable which I _thought_ would interrupt the cycling code. But that's not happening. — TonyLuigiC, Apr 15 '17 at 06:17

score 0 · Answer 1 · answered Apr 15 '17 at 06:03

0

no need to call function inside the timeout, just give the reference

setTimeout(window[callFunction](), mSeconds); should be setTimeout(window[callFunction], mSeconds);

answered Apr 15 '17 at 06:03

icebug

193
4

score 0 · Answer 2 · answered Apr 15 '17 at 06:38

Just gonna throw this out there as a different approach to the whole thing. You could easily add something to interrupt the while loop or make it advance only after the user clicks a particular element

const loadImage = url =>
  new Promise((resolve, reject) => {
    let img = document.createElement('img')
    img.src = url
    img.onload = event => resolve(img)
    img.onerror = err => reject(err)
  })
  
const displayImage = ({elem, images, index, ms}) =>
  new Promise(resolve => {
    elem.src = images[index].src
    setTimeout(resolve, ms, {elem, images, index: (index + 1) % images.length, ms})
  })
    
const slideshow = (elem, urls) =>
  Promise.all(urls.map(loadImage))
    .then(images => ({ elem, images, index: 0, ms: 1000 }))

async function playSlideshow (x) {
  while (true)
    x = await displayImage(x)
}

const logError = err => console.error(err.message)

// demo
const elem = document.querySelector('#slideshow img')

const imageUrls = [
  'http://placehold.it/200?text=A',
  'http://placehold.it/200?text=B',
  'http://placehold.it/200?text=C',
  'http://placehold.it/200?text=D',
  'http://placehold.it/200?text=E'
]

slideshow(elem, imageUrls)
  .then(playSlideshow, logError)

<div id="slideshow">
  <img>
</div>

Viliam Aboši · Accepted Answer · 2017-04-15T11:01:01.193

-1

What you are doing seems to be quite complicated. I suggest you rewrite it in following manner:

var runningTimeout = null;
var minImage = 1;
var maxImage = 5;
var currentImage = minImage;
var timerMiliseconds = 5000;

function cycler() {
    //In case this method is called from "outside" (prev/next) we clean the timeout.
    //This way there are no stray timeout that would cause unwanted behavior.
   if(runningTimeout != null) {
        clearTimeout(runningTimeout);
        runningTimeout = null;
  }
  //This part provides the cycling. If previous on first image sould display the last image,
  //divide the "if" in two ifs -> if currentImage < minImage then currentImage = maxImage.
  if(currentImage > maxImage || currentImage < minImage){
    currentImage = minImage;
  }
    console.log(currentImage++); //here display the image
  //Set the timeout again for the next iteration with the next image.
  runningTimeout = setTimeout(cycler, timerMiliseconds );
 }

 function prev() {
    //-2 because cycler did currentImage++ -> Displayed image = 4, currentImage = 5,
    //previous image = 3 => current (5) - 2 = previous (2)
    currentImage -= 2;
    cycler();
    //To stop cycling...
    if(runningTimeout != null) {
        clearTimeout(runningTimeout);
        runningTimeout = null;
    }
 }

 function next() {
    //cycler itself does currentImage++. Therefore there is no need to increment, just call 
    //it again for the currently set currentImage.
    cycler();
    //To stop cycling...
    if(runningTimeout != null) {
        clearTimeout(runningTimeout);
        runningTimeout = null;
    }
 }

As for the images, I suggest you load them once and have them hidden instead of creating the image elements "on the fly" every time. That way you would just set their visibility instead of rewriting the inner html...

edited Apr 15 '17 at 11:01

answered Apr 15 '17 at 06:39

Viliam Aboši

447
2
14

Thank you Viliam Aboši I'll keep that in mind. I have next() working now (somewhat), except that the cycling ever stops. I expected the slide show to grind to a halt. Any thoughts on that? – TonyLuigiC Apr 15 '17 at 06:47
cycling **n**ever stops – TonyLuigiC Apr 15 '17 at 06:54
@TonyLuigiC as to the original question I thought it should never stop. When do you want to halt it? Nevertheless, if you want to halt it all you need to do is `clearTimeout(runningTimeout); runningTimeout = null;` – Viliam Aboši Apr 15 '17 at 06:56
I want it to stop when I click the Back or Next button. I think you just posted what I needed, many thanks. – TonyLuigiC Apr 15 '17 at 07:35
@ Viliam Aboši One strange thing here: I changed function next() so it would call cycler() and interrupt the image cycling. function next() { nextClicked = 1; cycler(); } I also tried function next() { nextClicked++; cycler(); } However, in both cases, the variable value stays at zero. I've watched it in the Chrome Inspector, that line is read but ignored as if it doesn't exist. Do you have any idea as to what would cause this? – TonyLuigiC Apr 15 '17 at 10:50
@TonyLuigiC I would need to see your code, to figure out why it is not changing. My guess is, that you have it in a different scope (it is not global). I edited in the part to stop cycling in the next/previous functions... – Viliam Aboši Apr 15 '17 at 11:03
That's the strange part, it's right at the top of my page, right after – TonyLuigiC Apr 15 '17 at 11:11
@ Viliam Aboši here's the code: https://jsfiddle.net/qvjsvk9o/1/ And yes, I know it desperately needs to be cleaned up, but it's 7:25 am and I haven't slept yet, so cleanup will have to wait until this afternoon! Thank you. – TonyLuigiC Apr 15 '17 at 11:24
@TonyLuigiC I have rewritten the fiddle using my code: https://jsfiddle.net/nseb5qgs/1/. Hopefully it does what you need. Cycles through the images until previous image, or next image are clicked. As to your code: I do not know when are you checking the `nextClicked` variable. It could be zero, because you are rewriting it to zero (on line 49 of your fiddle). You are also doing `for (j = 0; j < setTimers; j++){clearTimeout(j);}`. This will work by chance only. The timeout id could be higher than 9. It could be easily 105 for the first image. You should store it in variable for later reference. – Viliam Aboši Apr 15 '17 at 21:30
@ Viliam Aboši **Thank you very much** for your help! – TonyLuigiC Apr 16 '17 at 07:03
Those who downvote based on duplicate questions really need to ask themselves **first** if the problem was clear enough to word the search correctly to **find** the original question. I didn't know that the method was being executed immediately when I used timeout, thus, I did not find the original question in my search. All that you accomplish is to chase away potential new members, which I doubt is the goal of this (or any other) website. – TonyLuigiC Apr 16 '17 at 07:08

Why is this JavaScript Loop not Working?

3 Answers3