# Slot Machine code critique

Ok, so this is the first program I have written in arduino (I used an arduino uno). It is supposed to be a slot machine thing that has the led’s (the led’s are used as the sets of slots) hooked up to pins 2-4, pins 6-8, and pins 10-12. I know you guys might not want to take the time to copy the program and hook it up to your arduinos but I was just wondering if I could get some tips. I know it is not written well and I was just wondering if I could get some general tips on better programming skills. Here is the code:

``````  int wincheck=0;
int count=0;
int rundelaymax;
int leftstop;
int midstop;
int rdmone = 0;
int rundelay = 50;
int leftleds = 5;
int midleds = 9;
int countnum = 0;

void setup() {

//set each pins used for the slot machine as outputs (pins 2-4, pins 6-8, pins 10-12)
for(int pinsout = 2;pinsout<5;pinsout++){
pinMode(pinsout, OUTPUT);
pinMode(pinsout+4, OUTPUT);
pinMode(pinsout+8, OUTPUT);
}

}

// here is the meat and potatoes of the program
void loop() {

//assign random numbers to each set of led's to make them stop (only assign them the first time the program gets run)
rdmone +=1;
if(rdmone==1){
leftstop = random(17,20);
midstop = random(20,23);
rundelaymax = random(600,700);
}

//start system that has led's running up like a slot machine
for(int rightleds=2;rightleds<5;rightleds+=1){
leftleds+=1;
midleds+=1;
rundelay+=20;
countnum+=1;

//right set of led's (pins 2-4) turn on (during the running of the slot machine)
digitalWrite(rightleds, HIGH);

//left set of led's (pins 6-8) turn on (during the running of the slot machine)
if(countnum>leftstop){}
else
digitalWrite(leftleds, HIGH);

//middle set of led's (pins 10-12) turn on (during the running of the slot machine)
if(countnum>midstop){}
else
digitalWrite(midleds, HIGH);

delay(rundelay);              //  delay before moving up to next LED

//right set of led's (pins 2-4) turn off (during the running of the slot machine)
digitalWrite(rightleds, LOW);

//left set of led's (pins 6-8) turn off (during the running of the slot machine)
if(countnum>(leftstop-1)){}
else
digitalWrite(leftleds, LOW);

//middle set of led's (pins 10-12) turn off (during the running of the slot machine)
if(countnum>(midstop-1)){}
else
digitalWrite(midleds, LOW);

//stop running of slots when all slots have stopped running
if(rundelay>rundelaymax){
while(1){
digitalWrite(rightleds, HIGH);

//check if you are a winner
for(wincheck=2;wincheck<5;wincheck+=1){

//        blink all led's on an off with a delay of 1 second in between each blink
delay(600);
for(count=0;count<3;count+=1){
for(wincheck=2;wincheck<5;wincheck+=1){
digitalWrite(wincheck,HIGH);
digitalWrite((wincheck+4),HIGH);
digitalWrite((wincheck+8),HIGH);
}
delay(1000);
for(wincheck=2;wincheck<5;wincheck+=1){
digitalWrite(wincheck,LOW);
digitalWrite((wincheck+4),LOW);
digitalWrite((wincheck+8),LOW);
}
delay(1000);
}
while(1){}
}
}

//blink an x in the led's with a delay of .5 seconds in between each blink to indicate that you lost
for(count=0;count<3;count+=1){

delay(500);

digitalWrite(2,HIGH);
digitalWrite(4,HIGH);
digitalWrite(7,HIGH);
digitalWrite(10,HIGH);
digitalWrite(12,HIGH);
digitalWrite(3,LOW);
digitalWrite(6,LOW);
digitalWrite(8,LOW);
digitalWrite(11,LOW);
delay(500);
digitalWrite(2,LOW);
digitalWrite(4,LOW);
digitalWrite(7,LOW);
digitalWrite(10,LOW);
digitalWrite(12,LOW);
}
while(1){}
}
}
}
if(leftleds>7){
leftleds=5;}
if(midleds>11){
midleds=9;}

}
``````

Running the code through the IDE's auto-format tool before posting would've been a bonus.

``````while (1) {}
``````

I see this 3 times in the program. You seem very determined to stop the program at various points but there seems to be no way of restarting it short of resetting or powering the Arduino on and off.

``````   pinMode(pinsout, OUTPUT);
pinMode(pinsout + 4, OUTPUT);
pinMode(pinsout + 8, OUTPUT);
``````

Instead of diddling around with +4 and +8 at various places can you not make the pin numbers contiguous or use an array of pin numbers ?.

Ok, so this is the first program I have written in arduino (I used an arduino uno). It is supposed to be a slot machine thing that has the led’s (the led’s are used as the sets of slots) hooked up to pins 2-4, pins 6-8, and pins 10-12. I know you guys might not want to take the time to copy the program and hook it up to your arduinos but I was just wondering if I could get some tips. I know it is not written well and I was just wondering if I could get some general tips on better programming skills.

The first thing that I’m always doing when starting programming is to think about:
How can I have a data structure that represents the physical circuit?

So I see, you are programming pins that have a true/false or on/off state:
That’s for me a data structure like that:

``````struct ledPin_t {const byte pin; boolean state;};
``````

Then you have 3 sets of 3 such LEDs. That’s for me a 2-dimensional array, organized in rows and colums:

``````#define NUMROWS 3
#define NUMCOLS 3
ledPin_t leds[NUMROWS][NUMCOLS]={
{{2,false},{3,false},{4,false}},
{{6,false},{7,false},{8,false}},
{{10,false},{11,false},{12,false}},
};
``````

So thats the data structure that my program would work with.

And working with that would be organized in functions that I write. Such like a function for initializing and logical resetting the LEDs:

``````void initAndResetLEDs()
{
for (int i=0;i<NUMROWS;i++)
{
for (int j=0;j<NUMCOLS;j++)
{
pinMode(leds[i][j].pin, OUTPUT); // set pinMode to OUTPUT
leds[i][j].state= LOW;  // set locical state to LOW/false
}
}
}
``````

Or a function to show the logical state of each LED on the physical pin:

``````void showOutput()
{ // write LED logical state to its pin
for (int i=0;i<NUMROWS;i++)
{
for (int j=0;j<NUMCOLS;j++)
{
digitalWrite(leds[i][j].pin, leds[i][j].state);
}
}
}
``````

And that way I would think about what functions the program would need, then I’d write those functions and finally the loop() function consists of some glue locic and functions calls as needed.

The first thing that I'm always doing when starting programming is to think about: How can I have a data structure that represents the physical circuit?

Unfortunately that is not how beginners think. Their primary objective is to get something working, maybe turning an LED on and off. When that works they add a second LED and maybe a third. Then they think about turning them on and off in sequence and adding more LEDs all based on the original LED on/off program and probably using delay().

At that point they should stop and think about what they are doing and probably start again as you describe but as the program that they have written nearly works and has grown to a couple of hundred lines of tangled code they are reluctant to do so and usually carry on down the slippery slope that they have started on.

Just a few more improvements, a tweak here and there and all will be well, but experienced programmers can see that it won't. Just think how many times the "replace delay() with millis()" situation arises.

OP - take heed of the advice that you have been given here, bite the bullet and rewrite the program using the techniques suggested.

Thanks for all of the advice guys. I greatly appreciate it. I know that this was horribly written and I have thought about rewriting the code. At this point I think I am going to start on another program and take the advice that you guys have given me. I will post my new program when I am finished and hopefully I will make some improvements although I am sure that there will still be many mistakes. Thanks again.