Arduino Snakes and Ladders

Here is a snakes and ladders game I made using an Arduino using a serial terminal, a set of addressable LED's to create a physical game board, a 7 segment display, and a piezoelectric to create sound.

I'd love to get some feed back on this regarding what I did right, what I did wrong, and any improvements that can be made to this game.

```
#include "FastLED.h"

#define NUM_LEDS 100

#define DATA_PIN 9

CRGB leds[NUM_LEDS];

//Assign correct pin values to corresponding variables
int piezoPin = 10;
int segmentOne = 7;
int segmentTwo = 2;
int segmentThree = 8;
int segmentFour = 4;
int segmentFive = 6;
int segmentSix = 3;
int segmentSeven = 5;

int boardArray[101]; // array of numbers index = value, depending on the value of the index determains what is in that square of the board

int snakeArray[7][1]; // array of 8 snakes, with a start and end value --- index starts from 0, 7 == 8
int ladderArray[7][1]; // array of 8 ladders, with a start and end value --- index starts from 0, 7 == 8

//Player Start Positions, the board array start from 0, we want to start from one, so set to 1.
int playerOnePosition = 1;
int playerTwoPosition = 1;

//variables needed to figure out which snake or ladder the player landed on, 10 is out of range and can be used as a null value
int ladderLandedOn = 10;
int snakeLandedOn = 10;

int turn = 1; // curren't player
boolean gamefinish; //boolean to setmode gam
int winner = 0; // game winner
int userInput; // int variable to store the users input
int gameMode; //variable to store the selected game mode

boolean gameStarted = false; // variable to store the game status, if false the game needs to wait for a game selection

void setup() {
randomSeed(analogRead(0)); // each loop randomise the seed, stops repetative number generation between games when the arduino isn't being reset

Serial.begin(38400); //Start the Serial Monitor
//Set the pin modes.
pinMode(piezoPin, OUTPUT);
pinMode(segmentOne, OUTPUT);
pinMode(segmentTwo, OU

Arduino specifics

Disclaimer: I've got very little experience with Arduino programming. That said, my understanding is that the code you write essentially sits inside a function like this:

setup();     // User provided function, executed once on startup
while(true) {
    loop();  // User provided function, executed over and over again
}

Assuming I'm correct, then it looks like you have a potential bug in your loop code. At the end of it you have a line that checks the current gameMode and if it's not 1,2 or 3 it calls loop.

(gameMode == 1 ? modeComputerVsComputer() : 
                (gameMode == 2 ? modeSinglePlayer() 
                               : (gameMode == 3 ? modeMultiplayer() 
                                                : loop())));

You don't need to do this, it's handled automatically for you. By calling loop yourself you're recursing, if you do it enough times, you're going to end up with a stack overflow. My guess is that the Arduino would just reset in this situation and then call setup again. Since you're not playing, you might not even notice it happen.

Your setup contains randomSeed(analogRead(0)). This is also the first line of your loop method. Setup doesn't seem to actually be using the randomiser, so I suspect you don't need to call it from setup, simply call it from loop.

Ternary Operator

I've got nothing against the ternary operator, however some of your choices do nothing but make your code harder to follow. Nested ?s should be avoided. Creating a null function so that you can use the ternary operator is unnecessarily complex.

The method would be easier to follow if you used constants and spread out the logic a bit.

randomSeed(analogRead(0)); 
if (!gameStarted) gameInit();
switch(gameMode)
{
    case GAME_MODE_COMPUTER_VS_COMPUTER:
        modeComputerVsComputer();
        break;
    case GAME_MODE_SINGLE_PLAYER:
        modeSinglePlayer();
        break;
    case GAME_MODE_MULTIPLAYER:
        modeMultiplayer();
        break;
}

Alternately you could setup an array of function pointers and index into it based on the gamemode to get the method to execute.

Repeated Code

There seems to be some repeated code between the different game modes. For example the following is repeated twice in each game mode (once for player 1 win, once for player 2). There is a slight variation in computerVsComputer, where the blinkRandomFade is omitted, it's unclear if this is intentional or not.

blinkRandomFade();
Serial.println("The winner is Player 1");
showEndGameMenu();
userInput = getUserInput();

This could be refactored into a method that takes the winning player number. There are other repeated blocks that again could be refactored into functions to reduce the overall amount of code and make it easier to follow the logic. For example, startGame?:

boardGen();
resetLights();
gameStarted = true;

The Interface

You've done some good centralisation of the interface with some of the board elements drawRoll, playSound centralise the interaction so that you could replace these methods and maintain the core logic of the game. However, your interaction with the Serial interface permeates your entire code. If you centralised this interaction, then it would be easier to remove the dependency (for example to run it on a computer, or if you wanted to use different components in the future).

Consider using function pointers

There are various points in your code where you are calling different methods based on an index like this:

if (randomNumber == 1)
{
    drawOne();
}
else if (randomNumber == 2)
{
    drawTwo();
}
else if (randomNumber == 3)
{
    drawThree();
}
else if (randomNumber == 4)
{
    drawFour();
}
else if (randomNumber == 5)
{
    drawFive();
}
else if (randomNumber == 6)
{
    drawSix();
}

Consider replacing this with function lookup table, that way the invocation is a lot simpler (bounds checking may be required, although not for this case because of the source of the number):

void (*drawNumberMethods[6])() = {drawOne,drawTwo,drawThree,drawFour,drawFive,drawSix};

drawNumberMethods[randomNumber-1]();

Magic Numbers

For the most part, I don't mind having actual numbers in code where they make sense in context. In the die drawing code above, it is easy to follow that the 1 and drawOne are connected as the value of the die. However some of your numbers are less clear, like ladderLandedOn = 10 representing not on a ladder. Where the meaning isn't obvious, avoid using the numbers and consider replacing them with a meaningful constant / #define.

StackExchange Code Review Q#129951, answer score: 10