Clean Up Code

Hi All,

New to arduino.

I have my code working just wondered if i could clean it up a little or implement it a different way.

int pbutton1pin = 2;// push button 1 pin
int relay1pin = 10;// relay 1 pin

int b1state = 0; // push value from button 1
int relay1 = 0;//Relay 1 status
int buttonpushed1 = 0;//button 1 push status


int pbutton2pin = 3;//  push button 2 pin
int relay2pin = 11;// relay 2 pin

int b2state = 0; // push value from button 2
int relay2 = 0;//Relay 2 status
int buttonpushed2 = 0;// button 2 push status


int PRIrelay1 = 4; // Pin connected to pri switch relay
int relay3pin = 12; // Relay 3 pin 

void setup() {
  Serial.begin(9600);
  pinMode(pbutton1pin, INPUT_PULLUP); 
  pinMode(relay1pin, OUTPUT);
   pinMode(pbutton2pin, INPUT_PULLUP); 
  pinMode(relay2pin, OUTPUT);
    pinMode(PRIrelay1, INPUT_PULLUP);
  pinMode(relay3pin, OUTPUT);


}

void loop() {

 

  b1state = digitalRead(pbutton1pin);// Read Button 1 Push value
  b2state = digitalRead(pbutton2pin);// Read Button 2 Push value

  
//  Start of Relay 1 ********** /  
  if(b1state == LOW && relay1 == HIGH){

    buttonpushed1 = 1-buttonpushed1;
    delay(100);
  }    

 
  relay1 = b1state;

      if(buttonpushed1 == HIGH){
        Serial.println("Relay 1 ON");
        digitalWrite(relay1pin, LOW); 
       
      }else{
        Serial.println("Relay 1 OFF");
        digitalWrite(relay1pin, HIGH);
   
      }   
  
//  End of Relay 1 ********** /  

//  Start of Relay 2 ********** /  

 
  if(b2state == LOW && relay2 == HIGH){

    buttonpushed2 = 1-buttonpushed2;
    delay(100);
  }    

  relay2 = b2state;

      if(buttonpushed2 == HIGH){
        Serial.println("Relay 2 On");
        digitalWrite(relay2pin, LOW); 
       
      }else{
        Serial.println("Relay 2 Off");
        digitalWrite(relay2pin, HIGH);
   
      }   
          

//  End of Relay 2 ********** /  

// Start Of PRI Relay 1 ***********/
 int PRIread1 = digitalRead(PRIrelay1); // Read the state of the PRI Relay

   

  
     if (PRIread1 == LOW) // If the pin reads low, the Pri Relay is Active.
  {
    Serial.println("PRI Relay Active");
    digitalWrite(relay3pin, LOW); // Turn the relay on
  }
  else
  {
    digitalWrite(relay3pin, HIGH); // Turn the Relay off
  }

  //End Of PRI Relay 1 *************/
delay(100);
}

Thanks :slight_smile:

Regards
Mason

You could change the pin numbers to constant eight bit types.
You could move the variables that don't need to be globals to local scope.
You could not explicitly initialise global variables that don't need initialising.

You could use the F() macro to keep string literals in flash.

As a simple tidy-up method, you could use the auto-format tool in the IDE

You could use arrays for the pin numbers, pin states, pin statuses, etc.

The pins used should be constants and bytes or defined '#', a const uses less memory, as said prior use the F macro on Serial print statements and lastly delay is undesirable mills would be better allowing the system to do other tasks

Yes, use to format your code.

Suggest you place { and } on lines by themselves.

larryd:
Suggest you place { and } on lines by themselves.

I go back and forth on this one. I learned C using K&R and their style is not to have braces on their own line. I like that because I get to see more code on a given display without scrolling. On the other hand, braces on their own line does make a more clear delineation of code blocks. The fact that the IDE marks the start-end of blocks when the cursor sits near the brace lessens the second argument, to me. Other than for function signatures, I'll probably follow the K&R style