code review: yet another simon game

Projects Showcase
1

That's my approach to the classic simon game. It's quite different from other implementations seen in this forum. I'd like to know what you think about (errors, suggestions, improvements).

Thanks!

/* simon game
 *
 * player plays 10 turns
 * LED flash and restart (same sequence) on error
 * reset to get a new sequence
 * no sound (yet)
 *
 * 26/5/2013 - Leonardo Canducci
 */

//  LED pins
int led[] = { 2, 3, 4, 5};
// button pins
int button[] = { 8, 9, 10, 11};
// turn
int turn = 1;
// random LED sequence
int sequence[10];

void setup(){
  // LED pins
  for(int x = 0; x < 4; x++){
    pinMode(led[x], OUTPUT);
  }
  // button pins
  for(int x = 1; x < 4; x++){
    pinMode(button[x], INPUT);
  }

  // serial debugging
  Serial.begin(9600);

  // sequence generation (10 numbers from 0 to 3)
  randomSeed(analogRead(0));
  for(int x = 0; x < 10; x++){
    sequence[x] = random(0, 4);
  }
}

void loop(){
  // 10 turns (restart on error)
  while(turn < 10){
    // debug
    Serial.print("turn ");
    Serial.println(turn);
    // function that shows the LED sequence
    showSequence();
    // function to get player input and check for errors
    playerInput();
    // next turn one more LED 
    turn = turn + 1;
    // pause
    delay(2000);
  }
}

void showSequence(){
  // turn on and off a number of LEDs equal to turn variable
  for(int x = 0; x < turn; x++){
    digitalWrite(led[sequence[x]], HIGH);
    delay(500);
    digitalWrite(led[sequence[x]], LOW);
    delay(500);
    // debug
    Serial.print(sequence[x]);
  }
  Serial.println();
}

void playerInput(){
  // check a number of buttons equal to turn
  for(int x = 0; x < turn; x++){
    Serial.println("waiting for input");
    // waits for a button press (0 = not pressed, 1 = pressed)
    int buttonPressed = 0;
    while(buttonPressed == 0){
      // check the 4 buttons
      for(int y = 0; y < 4; y++){
        // on button press
        if(digitalRead(button[y]) == HIGH){
          // debug
          Serial.print("button ");
          Serial.print(y);
          Serial.println(" pressed");
          // turn on the matching LED
          digitalWrite(led[y], HIGH);
          delay(200);
          digitalWrite(led[y], LOW);
          delay(200);
          // on error
          if(y != sequence[x]){
            Serial.println("wrong button!");
            // flash the LEDs
            for(int i = 0; i < 4; i++){
              digitalWrite(led[i], HIGH);
            }
            delay(500);
            for(int i = 0; i < 4; i++){
              digitalWrite(led[i], LOW);
            }
            delay(500);           
            // reset turn and start over (same sequence) 
            // 
            turn = 0;
            // exit function
            return;
          }
          // store button pressed event
          buttonPressed = 1;
          // exit for loop (one of the four buttons pressed)
          // back to while loop
          break;
        }
      }
    }    
    // delay (debounce)
    delay(100);
  }
}
2 
int led[] = { 2, 3, 4, 5};

int button[] = { 8, 9, 10, 11};

Unless you're planning on changing these while the program is running, I'd make them "const", and also I'd make them "byte".

int buttonPressed = 0;
    while(buttonPressed == 0){

No big deal, but using sixteen bits to store one bit's-worth of data is wasteful.
Again, consider using "byte".

Consider also getting rid of all those "delay"s.

for(int x = 1; x < 4; x++){
    pinMode(button[x], INPUT);

Luckily, pins are INPUT by default, otherwise that could've caused some hair loss.

3

AWOL:

int led[] = { 2, 3, 4, 5};

int button[] = { 8, 9, 10, 11};

Unless you're planning on changing these while the program is running, I'd make them "const", and also I'd make them "byte".

int buttonPressed = 0;

while(buttonPressed == 0){



No big deal, but using sixteen bits to store one bit's-worth of data is wasteful.
Again, consider using "byte".

I know... I didn't because I'm planning to use that in a school course and I'm trying to simplify (datatype, constants).

Consider also getting rid of all those "delay"s.

I don't get it. Except for the last one I'm using delay() to keep the LEDs on for a fixed amount of time.

for(int x = 1; x < 4; x++){

pinMode(button[x], INPUT);



Luckily, pins are INPUT by default, otherwise that could've caused some hair loss.

Whoops! fixed. If INPUT is default what's the point of using pinMode() in all the code examples?

One last thing: what about the actual implementation? Is there a more clean/obvious way to do it?

4

I didn't because I'm planning to use that in a school course

All the more reason to start good habits early.

If INPUT is default what's the point of using pinMode() in all the code examples?

Again, good habits, and it makes things clear.

Except for the last one I'm using delay() to keep the LEDs on for a fixed amount of time

you have delays totalling 400milliseconds during playerInput, which could make things a tad unresponsive.