Feedback on code

Hello all!

I decided to make a little lock box with an Arduino today. Currently it consists of a 2x4 button matrix, a servo and two status LEDs.

I was able to whip up the following code:

#include <Keypad.h>
#include <Servo.h>

// This defines the size of the keypad
/* Ex: This has 2 colums and 3 rows (+)
 *       +  +
 *       +  +
 *       +  +
 */
const byte rows = 4;
const byte cols = 2;

// These  pins connect the servo and the red and green status LEDs (duh)
const int ledPins[2] = {8, 9};
const int servoPin = A0;

// This is the secret to open the servo
const int secret[8] = {1, 1, 2, 2, 3, 4, 3, 4};

// This defines what the keys equate to
char hexaKeys[rows][cols] = {
  {'1','2'},
  {'3','4'},
  {'5','6'},
};

// This defines the pins connected to the Arduino
byte rowPins[rows] = {2, 3, 4};
byte colPins[cols] = {6, 7};

// This creates the keypad object
Keypad pad = Keypad( makeKeymap(hexaKeys), rowPins, colPins, rows, cols);

// This creates the servo object
Servo servoLock;

// This variable builds the array that is the combination
int usrVal[8] = {};
// This int tracks which part of the code the user is on
int valCount = 0;

void setup() {
  Serial.begin(9600);

  // This lets the Arduino know that which pins are dedicated to the LEDs and turns on the red status LED
  pinMode(ledPins[0], OUTPUT);
  pinMode(ledPins[1], OUTPUT);

  // This links the servo object to the corresponding pin, then sets it to a locked position
  servoLock.attach(servoPin);
  //servoLock.write(0);
  delay(500);
  updateStatus("l");
}

void loop() {
  // A intermediate variable to pick up the key pressed
  char customKey = pad.getKey();

  // If the Arduino picks up an input, it will log it in the Serial monitor
  if (customKey == '1'){
    overflowProtection();

    // This enters the user's input into the <valCount>th slot in the <usrVal> array
    usrVal[valCount] = 1;
    Serial.println("Up");
    
    // This ups the <valCount>
    valCount++;
  } else if (customKey == '2'){   // Rinse and repeat the first if statement to up to the third elif statement (where the condition is "customKey == '4'")
    overflowProtection();
    
    usrVal[valCount] = 2;
    Serial.println("Down");  
    
    valCount++;
  } else if (customKey == '3'){
    overflowProtection();
    
    usrVal[valCount] = 3;
    Serial.println("Left");  
    
    valCount++;
  } else if (customKey == '4'){
    overflowProtection();
    
    usrVal[valCount] = 4;
    Serial.println("Right");  
    
    valCount++;
  } else if (customKey == '5'){  // This if statement acts as the enter key
    Serial.println("Enter");
    // This conditional will update the status LED to an unlocked status if the user's combination equals the predefined secret and set the servo into an unlocked position
    if ( array_cmp(secret, usrVal, 8, 8) == true){
      updateStatus("u");
    }

    // This resets the user's combination
    reset();
  } else if (customKey == '6'){
    Serial.println("Lock");

    // This updates the status LED and servo into an unlocked position
    updateStatus("l");

    // This resets the user's combination
    reset();
  }

}


// This function resets the user's combination, enters it into the Serial monitor for debugging, then resets the combination's positional counter
void reset() {
  for (int i = 0; i < 8; i++){
      usrVal[i] = 0;  
  }
    
  for (int i = 0; i < 8; i++){
      Serial.print(String(usrVal[i]) + " ");  
  }

  valCount = 0;
}


// This is a function that updates the status LEDs to a locked or unlocked status depending on the parameter
void updateStatus(String S) {
  if (S == "l") {
    digitalWrite(ledPins[0], HIGH);
    digitalWrite(ledPins[1], LOW); 
    servoLock.write(90); 
  } else if (S == "u") {
    digitalWrite(ledPins[1], HIGH);
    digitalWrite(ledPins[0], LOW);
    servoLock.write(0);
  }
}


// This function will wipe the user's combination if they enter too many inputs (differs from reset for readability)
void overflowProtection() {
  if (valCount == 8){
    for (int i = 0; i < 8; i++){
      usrVal[i] = 0;  
    }
  }  
}


// A function I found on the Arduino forum to compare two arrays, position by position 
// All credit and appreciation goes to ShadowX!!
boolean array_cmp(int *a, int *b, int len_a, int len_b){
     int n;

     // if their lengths are different, return false
     if (len_a != len_b) return false;

     // test each element to be the same. if not, return false
     for (n=0;n<len_a;n++) if (a[n]!=b[n]) return false;

     // ok, if we have not returned yet, they are equal :)
     return true;
}

I know that it currently works but, since I haven’t had too much time to code recently, I was hoping for some feedback. What could be done better and in a more efficient way?

Thank you! :slight_smile:

ArnavPawarAA:
You can only make a new post only when you are having any problem, issue or inconvenience.

where is that rule written in the forum’s netiquette?

For your code, it is fine and good to go. : )

There are always constructive remarks that can be made.

Any good reason for going slowly? Serial.begin(9600);

The code could be improved by declaring const stuff as const, using bytes instead of int where possible, using == true where bool are tested in the code is not needed, it’s already a truth type… better Formal type coherence always helps.

Also you might consider using a switch case instead of nested if/else, it helps with readability and might be a bit faster at execution (at the expense of a bit of memory for a jump table the compiler will build).

The whole data entry could be simplified as you just store in your array the values from the keys. Probably no need at all for the if/else in the first place. And you could use the actual values in the keypad array description so that you don’t have to transform the char into your int (which should be a byte) or use char in your passcode definition.

Using The Whole String class when you only need one char testing in void updateStatus(String S) { is overkill and pissing out memory and cpu cycles for no gain. Just pass a char as argument and use single quotes when calling the function.

Regarding overflowProtection(), There is the memset() function to reset the data into an array, no need to reinvent your own fonction. Don’t you have a need to reset valCount once the array has been cleared? And for extra safety test against >= 8 instead of == 8, that will capture more possible errors you might have in the code.

Using magic numbers like 8 is not good, you should define a const at the start of the program by calculating the max size of your array or just defining it as the max size and use it for the array declaration. Replace then every relevant 8 with your defined constant.

Those two for loops

 for (int i = 0; i < 8; i++){
      usrVal[i] = 0;  
  }
    
  for (int i = 0; i < 8; i++){
     Serial.print(String(usrVal[i]) + " ");  
  }

could be merged and no need for the string class and you know what’s Printed

 for (int i = 0; i < 8; i++) {
  usrVal[i] = 0;
  Serial.print(F(“0 “));
}

But I would do just

memset(usrVal, 0, maxKeys); // maxKeys would be the constant byte for 8, the size of the array of char/byte instead of int
Serial.println(F(“User code Reset.”))

Just some ideas

memcmp is a useful function that you don’t have to debug yourself, at the slight expense of having to handle different size arrays yourself (not difficult)

Also, if “rows” has the value 4, it would be useful to comment why arrays that use this constant in their dimensions are not fully initialised.

I can’t see a good reason for a String, when an enum would do.

ArnavPawarAA:
No need for more improvements, but your problems are not sufficient for a new post.

I don’t understand this, apart from the post-count boost it provides.

TheMemberFormerlyKnownAsAWOL: I don't understand this, apart from the post-count boost it provides.

Agreed, it seems @ArnavPawarAA is really making very poor recommendations and bold affirmative statements that are ill guided... --> detrimental to the forum quality...

TheMemberFormerlyKnownAsAWOL: memcmp is a useful function that you don't have to debug yourself, at the slight expense of having to handle different size arrays yourself (not difficult)

true. I was suggesting to change the arrays to byte or char, hence I byte per entry.

TheMemberFormerlyKnownAsAWOL: I can't see a good reason for a String, when an enum would do.

I hesitated about that one, would have to be a typed enum (to uint8_t) otherwise you get 2 bytes as the default type for an enum is an int. I felt that was going to be a long explanation and 'u' or 'l' were good enough on one byte.

but yes, enum : byte {whateverUmeans, whateverLmeans};would be a great addition