Simon Game Flashing Button Problem with Array

1

I’ve been stuck on trying to write this loop for a couple days now.

Notes:
buttonFlash(color) // The color is passed in by a random number generator (1=green, 2=red, 3=blue, 4=yellow)
flashColor is a color which should flash for 1 second before returning to the original color
cpuPattern is an array which holds the colors, the random generator pushes the random color string to the end of the array ex: cpuPattern.push(‘green’); before running the buttonFlash(color) function

So in my head the for loop starts at the first element of the array which is a color string ex: ‘green’, gets the elementById(cpuPattern[i] which is ‘green’ which is the id of the green button), flashes the associated color, then after 1 second setTimeout gets the elementById(cpuPattern[i] which is ‘green’) and resets it to cpuPattern[i] which is ‘green’. The first part works, it sets the button to the flashColor but the second part never happens. I can’t figure out why. I’m trying to go through the whole array with the for loop, ex: cpuPattern[‘green’, ‘red’, ‘blue’, ‘red’] should loop 4 times and flash the green button then reset it after 1 second, flash the red button then reset it after 1 second, flash the blue button then reset it after 1 second, flash the red button then reset it but it is not working. Also - without the for loop it works fine, the correct color flashes and then resets but the game is only running with the last element because it’s not looping through the array.

function buttonFlash(color) {
  var flashColor = '';
  switch (color) {
    case 'green':
      flashColor = 'lightgreen';
      break;
    case 'red':
      flashColor = 'pink';
      break;
    case 'blue':
      flashColor = 'lightskyblue';
      break;
    case 'yellow':
      flashColor = 'cornsilk';
      break;
    }
  for (var i = 0; i < cpuPattern.length; i++) {
      document.getElementById(cpuPattern[i]).style.backgroundColor = flashColor;
  setTimeout(function() {
    document.getElementById(cpuPattern[i]).style.backgroundColor = cpuPattern[i];
  }, 1000);
  }
}

The codepen link is:

Pressing the dark red button starts the game.

2

This isn’t working because by the time setTimeout's callback function fires, i is no longer pointing to the correct index. You need to cache that value in order to use it when setTimeout fires. Something like

setTimeout((function(index) {
    return function() {
         document.getElementById(cpuPattern[index]).style.backgroundColor = cpuPattern[index];
    }
  })(i), 1000);

I haven’t really tested this code, so it might not work as-is, but the idea is to create a closure with an IIFE. A simpler solution is to use the let keyword instead of var for your loop declaration.

for (let i = 0; i < cpuPattern.length; i++) {
    //stuff
}

This should work in most browsers, but it’s still best to use the Babel precompiler anyways.

3

Tip : Make your functions shorter, make them do less things and whenever possible, make their behaviour not modify other parts of your program.This way, you can reason about them better, debug and reuse them easily, just like your playSound function.This is especially useful in programs that involve not so simple logic like Simon Game.Give your workers an explicit name, make them do one specific task, and you will solve bigger tasks faster and with less effort.Good luck.

function getRandomColor() {
  var randomNumber = Math.floor(Math.random() * 4);
  var colorPool = ['green', 'red', 'blue', 'yellow'];
  return colorPool[randomNumber];
}

function fillCpuPattern() {
  for (var i = 0; i < 4; i++) {
    cpuPattern.push(getRandomColor());
  }
}

function flashOneButton(color) {  
  // ...
}

function flashAllButtons() {
  for (var i = 0; i < cpuPattern.length; i++) {
    flashOneButton(cpuPattern[i]);
  }
}
4

Great, thanks for the replies! Let worked but I’ll rewrite the code and try to apply what you said. I don’t really know what let does so I’d rather not use it at the moment.