Go Down

Topic: code review: yet another simon game (Read 1 time) previous topic - next topic

arod0405

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!

Code: [Select]

/* 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);
  }
}

AWOL

Code: [Select]
int led[] = { 2, 3, 4, 5};
Code: [Select]
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".

Code: [Select]
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.

Code: [Select]
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.

arod0405


Code: [Select]
int led[] = { 2, 3, 4, 5};
Code: [Select]
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".

Code: [Select]
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).

Quote

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.

Quote

Code: [Select]
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?

AWOL

#3
May 30, 2013, 10:12 pm Last Edit: May 30, 2013, 10:15 pm by AWOL Reason: 1
Quote
I didn't because I'm planning to use that in a school course

All the more reason to start good habits early.

Quote
If INPUT is default what's the point of using pinMode() in all the code examples?
Again, good habits, and it makes things clear.

Quote
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.

Go Up