Sketch optimization?

Hi everyone…I recently modified an existing sketch so that you can input a passcode by pressing 3 buttons, where each button allows you to scroll through the numbers 1-9. After setting the correct code (2-4-1), the led changes/flashes. There is also an lcd attached with some prompts and a small servo just for the heck of it, which moves to a certain position based on whether the code is correct or not.

The sketch is simple and works for what I wanted it to do. However, being a newbie, I was wondering if anyone had some advice on optimization/simplification for future projects.

Would be grateful for any suggestions.

Thx!

Password2.ino (3.92 KB)

So you don’t have to download the file:

 #include <Servo.h>
#include <LiquidCrystal.h>
LiquidCrystal lcd(11, 10, 6, 7, 8, 9);
Servo myServo;
const int buttonPin1 = 2;   //buttons 1, 2 and 3
const int buttonPin2 = 4;
const int buttonPin3 = 5;
const int ledPinGreen = 12; 
const int ledPinRed = 13;

// Variables will change:
int buttonPushCounter1 = 0;  
int buttonPushCounter2 = 0;
int buttonPushCounter3 = 0;
int buttonState1 = 0;
int buttonState2 = 0; 
int buttonState3 = 0; 
int lastButtonState1 = 0; 
int lastButtonState2 = 0;
int lastButtonState3 = 0;

void setup() {
  myServo.attach(3);
  lcd.begin(16,2);
  lcd.print("Enter the");
  lcd.setCursor(0,1);
  lcd.print("code!");
  pinMode(buttonPin1, INPUT);
  pinMode(buttonPin2, INPUT);
  pinMode(buttonPin3, INPUT);
  pinMode(ledPinGreen, OUTPUT);
  pinMode(ledPinRed, OUTPUT);
  digitalWrite(ledPinRed, HIGH); //a little led flashing
  digitalWrite(ledPinGreen, HIGH);
  delay(100);
  digitalWrite(ledPinRed, LOW);
  digitalWrite(ledPinGreen, LOW);
  delay(100);
  Serial.begin(9600);
  digitalWrite (ledPinRed, HIGH);
  digitalWrite (ledPinGreen, LOW);
  myServo.write(0);
}


void loop() {
  buttonState1 = digitalRead(buttonPin1); //button 1

  if (buttonState1 != lastButtonState1) {

    if (buttonState1 == HIGH) {
      buttonPushCounter1++;
      Serial.println("on");
      Serial.print("number of button1 pushes:  ");
      Serial.println(buttonPushCounter1);
    } else {
      Serial.println("off");
    }
    delay(50);
  }
  lastButtonState1 = buttonState1;

  buttonState2 = digitalRead(buttonPin2); //button 2

  if (buttonState2 != lastButtonState2) {
    if (buttonState2 == HIGH) {
      buttonPushCounter2++;
      Serial.println("on");
      Serial.print("number of button2 pushes:  ");
      Serial.println(buttonPushCounter2);
    } else {
      Serial.println("off");
    }
    delay(50);
  }
  
  lastButtonState2 = buttonState2;

  buttonState3 = digitalRead(buttonPin3); //button 3
  
  if (buttonState3 != lastButtonState3) {
    if (buttonState3 == HIGH) {
      buttonPushCounter3++;
      Serial.println("on");
      Serial.print("number of button3 pushes:  ");
      Serial.println(buttonPushCounter3);
    } else {
      Serial.println("off");
    }
    delay(50);
  }

  lastButtonState3 = buttonState3;

//lcd display while entering code
  if (buttonState1 >= 1 || buttonState2 >=1 || buttonState3 >=1) {
      lcd.clear();
      lcd.print("Entering...");
      lcd.setCursor(0,1);
      lcd.print(buttonPushCounter1);
      lcd.print("-");
      lcd.print(buttonPushCounter2);
      lcd.print("-");
      lcd.print(buttonPushCounter3);
  }
  //once each button is pushed 9 times, it starts again at 0 so you can scroll through the set of numbers
  if (buttonPushCounter1 == 10) {
    buttonPushCounter1 = 0;
  }
   if (buttonPushCounter2 == 10) {
    buttonPushCounter2 = 0;
  }
   if (buttonPushCounter3 == 10) {
    buttonPushCounter3 = 0;
  }

  if (buttonPushCounter1 == 2)  //the correct code
  if (buttonPushCounter2 == 4) 
  if (buttonPushCounter3 == 1) {
    delay(50);
    digitalWrite(ledPinGreen, HIGH);
    digitalWrite(ledPinRed, LOW);
    delay(50);
    lcd.clear();
    lcd.print("You did it!");
    myServo.write(50); //servo moves/points to :)
  } else {
    digitalWrite(ledPinGreen, LOW);
    digitalWrite(ledPinRed, HIGH);
    
  }

  //if you wanted to limit the input to a certain number of attempts 
  if (buttonPushCounter1 + buttonPushCounter2 + buttonPushCounter3 == 1000) {
    digitalWrite(ledPinRed, LOW);
    delay(500);
    digitalWrite(ledPinRed, HIGH);
    delay(500);
    digitalWrite(ledPinRed, LOW);
    delay(500);
    digitalWrite(ledPinRed, HIGH);
    lcd.clear();
    lcd.print("Wrong!"); //code incorrect after a certain number of attemps
    delay(1000);
    lcd.clear();
    lcd.print("Try again!");
    buttonPushCounter1 = 0;
    buttonPushCounter2 = 0;
    buttonPushCounter3 = 0;
    myServo.write(2000); //servo moves/points to :( then moves back to initial position
    delay(1000);
    myServo.write(0);
  }
}

Package it up into a library. That way it will be resuable for other projects. Think about how you might use this in the future, with just one single function call in loop() that calls checkPassword() or something like that.

For extra reusability, make it a class.

Don't optimize prematurely, and don't optimize when there is no benefit to doing so, except as an academic exercise. Optimization can sometimes make code harder to read; in this case you must consider if the improvement is worth the increased pain next time you need to look at the code (often it is not).

That caveat aside... You use int for the buttonState variables. These will only ever be 1 or 0. So make it a byte (or a boolean - but a boolean takes up a whole byte anyway) instead.

Related, you can test if those variables are zero or non-zero without a comparison operator. Like, (buttonState1 >= 1 || buttonState2 >=1 || buttonState3 >=1) could be (buttonState1 || buttonState2 || buttonState3) - any non-zero value (positive or negative) in an integer datatype is true, while zero is false.

When you're feeding string literals (text enclosed in quotes with nothing special about it) to print() statements, enclose them in F() macro, ie, Serial.println("on"); becomes Serial.println(F("on")); - this puts the string into flash only, thus saving ram (otherwise, the string literals are copied into ram on initialization). The F() macro only works for Serial.print()/println() and maybe lcd.print()/println() (don't know off the top of my head - if it doesn't work, it'll give a compile error) - if you need to do more advanced manipulation with a string from flash, it's more complicated, but F() covers a lot of common use cases.

The delay() calls are blocking. If you want your sketch to react to button presses while in those delays, you have to do soemthing different. See the articles/posts here on 'doing multiple things at once' - this is covered in literally hundreds of guides and articles, very common issue.

The variables where you count up button presses could be bytes, too (but think a moment before you do this. Say some day you want to make it count to a higher number. Will 255 be high enough? It looks like that's fine here - but it's a trivial example of how you could make your life harder as a result of optimization.

MorganS: Package it up into a library. That way it will be resuable for other projects. Think about how you might use this in the future, with just one single function call in loop() that calls checkPassword() or something like that.

For extra reusability, make it a class.

Awesome! Thanks for your suggestion. :)

DrAzzy: Don't optimize prematurely, and don't optimize when there is no benefit to doing so, except as an academic exercise. Optimization can sometimes make code harder to read; in this case you must consider if the improvement is worth the increased pain next time you need to look at the code (often it is not).

That caveat aside... You use int for the buttonState variables. These will only ever be 1 or 0. So make it a byte (or a boolean - but a boolean takes up a whole byte anyway) instead.

Related, you can test if those variables are zero or non-zero without a comparison operator. Like, (buttonState1 >= 1 || buttonState2 >=1 || buttonState3 >=1) could be (buttonState1 || buttonState2 || buttonState3) - any non-zero value (positive or negative) in an integer datatype is true, while zero is false.

When you're feeding string literals (text enclosed in quotes with nothing special about it) to print() statements, enclose them in F() macro, ie, Serial.println("on"); becomes Serial.println(F("on")); - this puts the string into flash only, thus saving ram (otherwise, the string literals are copied into ram on initialization). The F() macro only works for Serial.print()/println() and maybe lcd.print()/println() (don't know off the top of my head - if it doesn't work, it'll give a compile error) - if you need to do more advanced manipulation with a string from flash, it's more complicated, but F() covers a lot of common use cases.

The delay() calls are blocking. If you want your sketch to react to button presses while in those delays, you have to do soemthing different. See the articles/posts here on 'doing multiple things at once' - this is covered in literally hundreds of guides and articles, very common issue.

The variables where you count up button presses could be bytes, too (but think a moment before you do this. Say some day you want to make it count to a higher number. Will 255 be high enough? It looks like that's fine here - but it's a trivial example of how you could make your life harder as a result of optimization.

Thanks a bunch for all of your awesome feedback! My aim with the question was really about getting rid of any potentially superfluous steps or bytes, and your tips certainly did the trick.

I'm fine with the generally mundane nature of the sketch, just wanted to see if I was perhaps on the right track :)

By the way, I tried out F() for lcd.print and it worked out fine. Thanks again!

aahyatt:
By the way, I tried out F() for lcd.print and it worked out fine. Thanks again!

That’s because the LiquidCrystal library inherits from the abstract Print class provided by the Arduino cores. Serial does as well. If you want to make a class that can print the same way as Serial, inherit publicly from Print and implement the pure virtual function size_t write(uint8_t).

For the F() macro, the functions in use are the ones that take a __FlashStringHelper* as the argument.

    size_t print(const __FlashStringHelper *);
    size_t println(const __FlashStringHelper *);