Model Rocket project code Critiques

This is my code for firing 2 Model Rockets and moving 2 servos for angle when pressed ( 4 different push buttons to do each thing)
this is my first actual coding test. compiles fine, just looking for critiques and ways to make the code look better.
Thanks!

#include <Servo.h>
int button1 = 4; //button pin, SMALL SERVO connect to ground to move servo
int press1 = 0;
int button2 = 5; //button pin, BIG SERVO connect to ground to move servo 
int press2 = 0;
const int buttonPin1 = 2;     // SMALL SERVO the number of the pushbutton pin connected to ground 
const int buttonPin2 = 3;     // BIG SERVO the number of the pushbutton pin connected to ground 
const int RocketFire1 =  12;      // ROCKET 1 the number of the fire pin, but to be used with EasyVR later not a button
const int RocketFire2 =  11;      // ROCKET 2 the number of the fire pin, but to be used with EasyVR later not a button
int buttonState1 = 0;         // variable for reading the pushbutton status for rocket 1
int buttonState2 = 0;         // variable for reading the pushbutton status for rocket 2

Servo servo1;
Servo servo2;
void setup()
{
  pinMode(button1, INPUT);
  servo1.attach(7);
  digitalWrite(4, HIGH); //enable pullups to make pin high
  
   // initialize the rocket1 pin as an output:
  pinMode(RocketFire1, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin1, INPUT);    

  pinMode(button2, INPUT);
  servo1.attach(6);
  digitalWrite(5, HIGH); //enable pullups to make pin high
  
   // initialize the rocket1 pin as an output:
  pinMode(RocketFire2, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin2, INPUT);    
}

void loop()
{
    // read the state of the pushbutton value:
  buttonState1 = digitalRead(buttonPin1);

  // check if the pushbutton is pressed.
  // if it is, the buttonState is HIGH:
  if (buttonState1 == HIGH) {     
    // turn LED on:    
    digitalWrite(RocketFire1, HIGH);
  }
  else {
    // turn LED off:
    digitalWrite(RocketFire1, LOW); 
  }  
    
  press1 = digitalRead(button1);
  if (press1 == LOW)
  {
    servo1.write(160);
  }
  else {
    servo1.write(15);
  }

//SECOND SERVO AND ROCKET 2

    // read the state of the pushbutton value:
  buttonState2 = digitalRead(buttonPin2);

  // check if the pushbutton is pressed.
  // if it is, the buttonState is HIGH:
  if (buttonState2 == HIGH) {     
    // turn LED on:    
    digitalWrite(RocketFire2, HIGH);
  }
  else {
    // turn LED off:
    digitalWrite(RocketFire2, LOW); 
  }  
    
  press2 = digitalRead(button2);
  if (press2 == LOW)
  {
    servo2.write(100);
  }
  else {
    servo2.write(0);
  }
}

also i know running through the training on this site is great. but any good video training for Coding out there?

The first thing that I notice is that you use mixture of pin names and numbers.

  pinMode(button1, INPUT);
  servo1.attach(7);
  digitalWrite(4, HIGH); //enable pullups to make pin high

  // initialize the rocket1 pin as an output:
  pinMode(RocketFire1, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin1, INPUT);    

  pinMode(button2, INPUT);
  servo1.attach(6);
  digitalWrite(5, HIGH); //enable pullups to make pin high

  // initialize the rocket1 pin as an output:
  pinMode(RocketFire2, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin2, INPUT);

Meaningful names for pins are much easier to understand.

int button1 = 4; //button pin, SMALL SERVO connect to ground to move servo

So, is pin 4 an input button, as its name suggests, or something to do with a servo ? Same question for other pins with similar comments.

  pinMode(button2, INPUT);
  servo1.attach(6);
  digitalWrite(5, HIGH); //enable pullups to make pin high

An extreme case of mixing pin names and numbers. I would suggest that you use pinMode(button2, INPUT_PULLUP);and do away with the writing of HIGH to the pin. The parameter makes it very clear what state the input pin will be in.

How are the servos powered ? An external power supply for them is pretty much a prerequisite as the current available from the Arduino is limited and servos can be quite power hungry devices.

More generally I don't understand the role that the servos play in your project. Can you please expand on the sequence of actions and events needed to launch each rocket ?

What do the servos do ?

ok so cleaner pin numbers..

int button1 = 4; //button pin, SMALL SERVO connect to ground to move servo So, is pin 4 an input button, as its name suggests, or something to do with a servo ? Same question for other pins with similar comments.

these are the buttons to launch the rockets

so my void setup will look like

void setup()
{
  pinMode(button1, INPUT_PULLUP);
  servo1.attach(7);

   // initialize the rocket1 pin as an output:
  pinMode(RocketFire1, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin1, INPUT);    

  pinMode(button2, INPUT_PULLUP);
  servo1.attach(6);
  
   // initialize the rocket1 pin as an output:
  pinMode(RocketFire2, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin2, INPUT);    
}

????

There is a large high torque servo powered externally and a small micro servo (9g SG90) powered by the board. purpose to open a door and move the base. so 2 push buttons when pressed, number 1 (the small one) opens the door. The other one, when the button is pushed moves the base certain degrees then back.

the other to buttons launch the rockets

also just thought to take away the 1 button to open the door and just tie that in with the 2 "fire" buttons so that when either fire button is pressed it opens the door (waits till its open all the way) fires the Rocket. Waits 2 seconds then closes it by itself. not sure how to write that though......

I have added some comments

void setup()
{
  pinMode(button1, INPUT_PULLUP);
  servo1.attach(7);      //  <<<  Give the servo pin a name

  // initialize the rocket1 pin as an output:
  pinMode(RocketFire1, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin1, INPUT);  //  <<<  make this INPUT_PULLUP   

  pinMode(button2, INPUT_PULLUP);
  servo1.attach(6);      //  <<<  Give the servo pin a name

  // initialize the rocket1 pin as an output:
  pinMode(RocketFire2, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(buttonPin2, INPUT);  //  <<<  make this INPUT_PULLUP   
}

It is all very well giving pins names but please make sure that they are meaningful.
button2 and buttonPin2 are likely to be confused and don’t mean anything.
launchButton1 and doorButton1 (or similar as appropriate) would be better.
As you have so much duplicated code for the 2 rockets you should consider using functions to hold common code and/or using arrays to hold the pin data. Either of these will cut down the amount of code and using meaningful names for functions make the code easier to understand and maintain.

All this:

   // read the state of the pushbutton value:
  buttonState1 = digitalRead(buttonPin1);

  // check if the pushbutton is pressed.
  // if it is, the buttonState is HIGH:
  if (buttonState1 == HIGH) {     
    // turn LED on:    
    digitalWrite(RocketFire1, HIGH);
  }
  else {
    // turn LED off:
    digitalWrite(RocketFire1, LOW); 
  }

can be done in one line of code:

 digitalWrite(RocketFire1, digitalRead(buttonPin1)); 
// make pin RocketFire1 equal to the value of buttonPin1

So this is my more refined sketch. THANKS so much you guys this is amazingly helpful!
more critiques if necessary?

#include <Servo.h>
int SmallServoButton = 4; //button pin, SMALL SERVO connect to ground to move servo
int press1 = 0;
int BigServoButton = 5; //button pin, BIG SERVO connect to ground to move servo 
int press2 = 0;
const int FireRocket1 = 2;     // ROCKET 1s BUTTON the number of the pushbutton pin is on then connected to ground 
const int FireRocket2 = 3;     // ROCKET 2s BUTTONthe number of the pushbutton pin is on then connected to ground 
const int RocketPin1 =  12;      // ROCKET 1 the number of the fire pin, but to be used with EasyVR later not a button
const int RocketPin2 =  11;      // ROCKET 2 the number of the fire pin, but to be used with EasyVR later not a button
int buttonState1 = 0;         // variable for reading the pushbutton status for rocket 1
int buttonState2 = 0;         // variable for reading the pushbutton status for rocket 2

Servo DoorServo;
Servo servo2;

void setup()
{
  pinMode(SmallServoButton, INPUT);
  DoorServo.attach(7);
  digitalWrite(SmallServoButton, HIGH); //enable pullups to make pin high
  
   // initialize the rocket1 pin as an output:
  pinMode(RocketPin1, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(FireRocket1, INPUT);    

  pinMode(BigServoButton, INPUT);
  servo2.attach(6);
  digitalWrite(BigServoButton, HIGH); //enable pullups to make pin high
  
   // initialize the rocket1 pin as an output:
  pinMode(RocketPin2, OUTPUT);      
  // initialize the pushbutton pin as an input:
  pinMode(FireRocket2, INPUT);    
}

void loop()
{
    digitalWrite(RocketPin1, digitalRead(FireRocket1)); 
// make pin RocketFire1 equal to the value of buttonPin1


    //???????????????????????????????needs to stay open then close again when re pushed or when either button 1 or 2 for rockets?????????????????????????????????????
  press1 = digitalRead(SmallServoButton);
  if (press1 == LOW)
  {
    DoorServo.write(160);
  }
  else {
    DoorServo.write(15);
  }

//SECOND SERVO AND ROCKET 2
 digitalWrite(RocketPin2, digitalRead(FireRocket2)); 
// make pin RocketFire1 equal to the value of buttonPin1
  
  press2 = digitalRead(BigServoButton);
  if (press2 == LOW)
  {
    servo2.write(100);
  }
  else {
    servo2.write(0);
  }
}

also i feel like the beginning is also crowded?? or is it ok?

#include <Servo.h>
int SmallServoButton = 4; //button pin, SMALL SERVO connect to ground to move servo
int press1 = 0;
int BigServoButton = 5; //button pin, BIG SERVO connect to ground to move servo 
int press2 = 0;
const int FireRocket1 = 2;     // ROCKET 1s BUTTON the number of the pushbutton pin is on then connected to ground 
const int FireRocket2 = 3;     // ROCKET 2s BUTTONthe number of the pushbutton pin is on then connected to ground 
const int RocketPin1 =  12;      // ROCKET 1 the number of the fire pin, but to be used with EasyVR later not a button
const int RocketPin2 =  11;      // ROCKET 2 the number of the fire pin, but to be used with EasyVR later not a button
int buttonState1 = 0;         // variable for reading the pushbutton status for rocket 1
int buttonState2 = 0;         // variable for reading the pushbutton status for rocket 2

Servo DoorServo;
Servo servo2;

You now don’t need these lines:

int buttonState1 = 0;  // variable for reading the pushbutton status for rocket 1
int buttonState2 = 0;  // variable for reading the pushbutton status for rocket 2

Some style tips...

const int RocketPin1 =  12;      // ROCKET 1 the number of the fire pin, but to be used with EasyVR later not a button
const int RocketPin2 =  11;      // ROCKET 2 the number of the fire pin, but to be used with EasyVR later not a button

Rocket1Pin and Rocket2Pin, would be better. You have two rockets. Your chosen names would imply one rocket with two pins. Prefer using #define instead of const int for pin numbers and save some memory.

DoorServo.attach(7);

Define a name for the pin number to be consistent. I prefer defining pin names together in a section at the top of the sketch. Makes it easier to move wires and serves to document the I/O used by the sketch.

   // initialize the rocket1 pin as an output:
  pinMode(RocketPin1, OUTPUT);

Indent comments to the same level as code.

    digitalWrite(RocketPin1, digitalRead(FireRocket1)); 
// make pin RocketFire1 equal to the value of buttonPin1

Comment should before the line of code. Or at least be consistent in your style, always before, or always after.

    //???????????????????????????????needs to stay open then close again when re pushed or when either button 1 or 2 for rockets?????????????????????????????????????

Lose all the question marks, makes your comment harder to read. Try to keep comment width close to the code width, split comment lines as needed.

//SECOND SERVO AND ROCKET 2
 digitalWrite(RocketPin2, digitalRead(FireRocket2)); 
// make pin RocketFire1 equal to the value of buttonPin1

Copy/paste is your enemy. Fortunately only the comment is wrong here.

  if (press2 == LOW)
  {
    servo2.write(100);
  }
  else {
    servo2.write(0);
  }

Be consistent in your brace style. Also the above code could be written so...

 servo2.write(press2 == LOW ? 100 : 0);

You might get different opinions on which is more readable, but the latter avoids duplicated code.

  digitalWrite(BigServoButton, HIGH); //enable pullups to make pin high

Comments to the right of the code are usually harder to read.

int press1 = 0;
int BigServoButton = 5; //button pin, BIG SERVO connect to ground to move servo 
int press2 = 0;

press1 and press2 should not be global.

int press1 = 0; int BigServoButton = 5; //button pin, BIG SERVO connect to ground to move servo int press2 = 0;

press1 and press2 should not be global.

Not quite sure what you mean by "global"

If i wanted to add the rocket fire button to do this(below) how would I write that in code?? -Push rocket launch button -Moves Door servo open (to 95degs) -delay 1s (to make sure it opens) -Fire Rocket 1 -Delay 2 or 3 seconds -Close Door (move servo to 0degs)

that way i could remove the OpenDoor push button compeltely

StrikeForceInd:

int press1 = 0; int BigServoButton = 5; //button pin, BIG SERVO connect to ground to move servo int press2 = 0;

press1 and press2 should not be global.

Not quite sure what you mean by "global"

The variables have global scope, meaning they are visible anywhere in your program. http://www.learncpp.com/cpp-tutorial/42-global-variables/

StrikeForceInd:

If i wanted to add the rocket fire button to do this(below) how would I write that in code?? -Push rocket launch button -Moves Door servo open (to 95degs) -delay 1s (to make sure it opens) -Fire Rocket 1 -Delay 2 or 3 seconds -Close Door (move servo to 0degs)

that way i could remove the OpenDoor push button compeltely

That's easy. Wait for the button to change from being not pressed to pressed, then inside the if code block for when that is true put the code to carry out the actions that you want. The crude way to do the timing is to use the delay() command. It will prevent any other actions taking place during the delay() but that should not be a problem in this case unless you want to fire two rockets at the same time.