How does my code look?

I’m really wanting to learn how to make my code as clean as possible. I’m going to introduce the code below into a project I’m working on. All it does now is control the beginnings of a LCD menu system. I’d like some advice on the direction I am going. It works well now but I’m open to suggestions on making it cleaner. In the end, this will have at least two main menus with several sub menus which I have not introduced into the code yet and I’m still not sure the direction I will take on that.

//include librarys
#include <Wire.h>
#include <LiquidCrystal_I2C.h>
#include <IRremote.h>

//defines
const int RECV_PIN = 4;
int mode = 1;
int vol = 123;
int menu = 0;
int sub = 0;
int menuLast = 0;

// Define IR Receiver and Results Objects
IRrecv irrecv(RECV_PIN);
decode_results results;


// Define LCD pinout
const int  en = 2, rw = 1, rs = 0, d4 = 4, d5 = 5, d6 = 6, d7 = 7, bl = 3;

// Define I2C Address - change if reqiuired
const int i2c_addr = 0x27;

LiquidCrystal_I2C lcd(i2c_addr, en, rw, rs, d4, d5, d6, d7, bl, POSITIVE);

void setup() {
  // put your setup code here, to run once:
  Serial.begin(115200);
  lcd.begin(16, 2);
  lcd.setCursor(2, 0);
  lcd.print("STAGE LIGHTS");
  lcd.setCursor(0, 1);
  lcd.print("MODE:");
  lcd.setCursor(6, 1);
  lcd.print(mode);
  lcd.setCursor(8, 1);
  lcd.print("VOL:");
  lcd.setCursor(13, 1);
  lcd.print(vol);
  irrecv.enableIRIn();
}

void loop() {
  //Serial.println(menu);
  if (menu > 2) {
    menu = 2;
  }

  if (menu < 0) {
    menu = 0;
  }

  if (irrecv.decode(&results)) {
    String k = String(getCode());
    if (menu < 1) {
      if (k == "1" or k == "2" or k == "3" or k == "4" or k == "5" or k == "6" or k == "7") {
        int value = k.toInt();
        mode = value;
        lcd.setCursor(6, 1);
        lcd.print(mode);
      } else if (k == "UP") {
        mode = mode + 1;
        if (mode > 7) {
          mode = 1;
        }
        lcd.setCursor(6, 1);
        lcd.print(mode);
      } else if (k == "DOWN") {
        mode = mode - 1;
        if (mode < 1) {
          mode = 7;
        }
        lcd.setCursor(6, 1);
        lcd.print(mode);
      }

      if (k == "FUNC/STOP") {
        menu = 1;
        if (menu != menuLast) {
          menuLast = menu;
          lcdSetup(menu, sub);
        }
      }
    }
    if (menu > 0) {
      if (menu >= 1 and k == "UP") {
        menu = menu + 1;
        if (menu != menuLast) {
          menuLast = menu;
          lcdSetup(menu, sub);
        }
      }
      if (menu <= 2 and k == "DOWN") {
        menu = menu - 1;
        if (menu != menuLast) {
          menuLast = menu;
          lcdSetup(menu, sub);
        }
      }
    }
  }
}

String getCode() {
  String value;
  switch (results.value) {
    case 0xFF6897:
      value = "0";
      break;
    case 0xFF30CF:
      value =  "1";
      break;
    case 0xFF18E7:
      value = "2";
      break;
    case 0xFF7A85:
      value = "3";
      break;
    case 0xFF10EF:
      value = "4";
      break;
    case 0xFF38C7:
      value = "5";
      break;
    case 0xFF5AA5:
      value = "6";
      break;
    case 0xFF42BD:
      value = "7";
      break;
    case 0xFF4AB5:
      value = "8";
      break;
    case 0xFF52AD:
      value = "9";
      break;
    case 0xFFA25D:
      value = "PWR";
      break;
    case 0xFF629D:
      value = "VOL+";
      break;
    case 0xFFA857:
      value = "VOL-";
      break;
    case 0xFFE21D:
      value = "FUNC/STOP";
      break;
    case 0xFF22DD:
      value = "BACK";
      break;
    case 0xFF02FD:
      value = "PLAY/PAUSE";
      break;
    case 0xFFC23D:
      value = "FWD";
      break;
    case 0xFFE01F:
      value = "DOWN";
      break;
    case 0xFF906F:
      value = "UP";
      break;
    case 0xFF9867:
      value = "EQ";
      break;
    case 0xFFB04F:
      value = "ST/REPT";
      break;
  }
  irrecv.resume();
  delay(100);
  return value;
}

void lcdSetup(int menu, int sub) {
  Serial.println("updated");
  switch (menu) {
    case 0:
      lcd.clear();
      lcd.setCursor(2, 0);
      lcd.print("STAGE LIGHTS");
      lcd.setCursor(0, 1);
      lcd.print("MODE:");
      lcd.setCursor(6, 1);
      lcd.print(mode);
      lcd.setCursor(8, 1);
      lcd.print("VOL:");
      lcd.setCursor(13, 1);
      lcd.print(vol);
      break;

    case 1:
      lcd.clear();
      lcd.setCursor(2, 0);
      lcd.print("EQ-1");
      lcd.setCursor(8, 0);
      lcd.print("BARS-2");
      lcd.setCursor(0, 1);
      lcd.print("LEDS-3");
      lcd.setCursor(7, 1);
      lcd.print("MODES-4");
      lcd.setCursor(15, 1);
      lcd.print("*");
      break;

    case 2:
      lcd.clear();
      lcd.setCursor(1, 0);
      lcd.print("STANDBY-1");
      break;
  }
}

I would not use int for cases where byte or unsigned byte would be sufficient.
I would use const for values that should not change.
I would not use String on a device with limited memory, which means most Arduinos.
I would not use the name “value” in multiple places. It gets confusing and the name “value” does not explain what it is for.

Not what you were looking for. Sorry.

F() macro for printing static strings. (And what he said - dynamically allocated strings are evil)

if (k == "1" or k == "2" or k == "3" or k == "4" or k == "5" or k == "6" or k == "7") {
        int value = k.toInt();

you can do this in reverse, if it is not an integer, .toInt() return 0; then you can test for modevalue>0 && modevalue<8
i would add this to function ‘LcdSetup()’ menu 10 or 12 whatever just to get it out of setup()

  lcd.begin(16, 2);
  lcd.setCursor(2, 0);
  lcd.print("STAGE LIGHTS");
  lcd.setCursor(0, 1);
  lcd.print("MODE:");
  lcd.setCursor(6, 1);
  lcd.print(mode);
  lcd.setCursor(8, 1);
  lcd.print("VOL:");
  lcd.setCursor(13, 1);
  lcd.print(vol);

I never put anything behind a closing brace} else if (k == "DOWN") {
To first return a String from getcode() then to decode the values from it an take action accordingly is double work, (and since it’s a String, double trouble…) it think you can just take the actions from getCode() and return the new menu level, then do LcdSetup() with that level etc… then your loop() will look really neat and tidy.

Thanks for the tips guys. I’m learning as I go for the most part. Memory management needs to take some priority in my sketches very quickly. I don’t wan’t to learn bad habits and have to break them later.

I’ll work on my sketch and repost here for more advice.

@Deva_Rishi,

I was thinkking the same thing on the getCode and lcdSetup functions. About to work on that now.