Code refactor

[ghoul]

Hello friends,

I am tasked with refactoring the following code so that is it is easily maintainable.

#include "libs.hpp"




//  ----------------------------------------------------------------------
//  Screen handling


LiquidCrystal lcd(8, 9, 4, 5, 6, 7);


int menuPage = 0;
std::string menuItems[] = {"START CAPTURE", "START SHOWCASE" ,"PRESETS",
                           "SET TRIGGER", "SETTINGS", "ABOUT"};
int maxMenuPages = round(((sizeof(menuItems) / sizeof(std::string)) / 2) + .5);


// Bitmap icons drawn on screen
std::byte downArrow[8] = { (std::byte)0b00100, (std::byte)0b00100,
                           (std::byte)0b00100, (std::byte)0b00100,
                           (std::byte)0b00100, (std::byte)0b11111,
                           (std::byte)0b01110, (std::byte)0b00100 };
std::byte upArrow[8]   = { (std::byte)0b00100, (std::byte)0b01110,
                           (std::byte)0b11111, (std::byte)0b00100,
                           (std::byte)0b00100, (std::byte)0b00100,
                           (std::byte)0b00100, (std::byte)0b00100 };


// This function will generate the 2 menu items that can fit on the screen.
// They will change as you scroll through your menu.
// Up and down arrows will indicate your current menu position.
static void mainMenuDraw() {
    lcd.clear();
    lcd.setCursor(1, 0);
    lcd.print(menuItems[menuPage]);
    lcd.setCursor(1, 1);
    lcd.print(menuItems[menuPage + 1]);
    if (menuPage == 0) {
        lcd.setCursor(15, 1);
        lcd.write(std::byte(2));
    } else if (menuPage > 0 and menuPage < maxMenuPages) {
        lcd.setCursor(15, 1);
        lcd.write(std::byte(2));
        lcd.setCursor(15, 0);
        lcd.write(std::byte(1));
    } else if (menuPage == maxMenuPages) {
        lcd.setCursor(15, 0);
        lcd.write(std::byte(1));
    }
}




//  ----------------------------------------------------------------------
//  Reacting to button presses


// This function is called whenever a button press is evaluated.
// The LCD shield works by observing a voltage drop across the buttons all
// hooked up to A0.
static int evaluateButton(int x) {
    int result = 0;
    if (x < 50) {
        result = 1;  // up
    } else if (x < 195) {
        result = 2;  // down
    }
    return result;
}


int readKey;


static void operateMainMenu () {
    int activeButton = 0;
    while (activeButton == 0) {
        int button;
        readKey = analogRead(0);
        button = evaluateButton(readKey);
        switch (button) {
        case 0: // When button returns as 0 there is no action taken
            break;
        case 1:
            button = 0;
            menuPage = menuPage - 1;
            menuPage = constrain(menuPage, 0, maxMenuPages);
            mainMenuDraw ();
            activeButton = 1;
            break;
        case 2:
            button = 0;
            menuPage = menuPage + 1;
            menuPage = constrain(menuPage, 0, maxMenuPages);
            mainMenuDraw ();
            activeButton = 1;
            break;
        }
    }
}




//  ----------------------------------------------------------------------
//  Top level 


void setup() {
    lcd.begin(16, 2);
    lcd.createChar(std::byte(1), upArrow);
    lcd.createChar(std::byte(2), downArrow);
}


void loop() {
    mainMenuDraw();
    operateMainMenu();
}


// To simulate the rest of the toolchain
int main () {
    setup ();
    while (true) {
        loop ();
    }
}

The best I could come up with is:

#include "libs.hpp"


// TODO
//
#define PIN_EIGHT 0x8
#define PIN_NINE 0x9
#define PIN_SEVEN 0x7
#define PIN_SIX 0x6
#define PIN_FIVE 0x5
#define PIN_FOUR 0x4


LiquidCrystal lcd(8, 9, 4, 5, 6, 7);
int menuPage = 0;


static void mainMenuDraw(std::string *menuItems, int menuPage, int maxMenuPages)
{
	lcd.clear();
	lcd.setCursor(1, 0);
	lcd.print(menuItems[menuPage]);
	lcd.setCursor(1, 1);
	lcd.print(menuItems[menuPage + 1]);


	if (menuPage == 0) {
		lcd.setCursor(15, 1);
		lcd.write(std::byte(2));
	} else if (menuPage > 0 and menuPage < maxMenuPages) {
		lcd.setCursor(15, 1);
		lcd.write(std::byte(2));
		lcd.setCursor(15, 0);
		lcd.write(std::byte(1));
	} else if (menuPage == maxMenuPages) {
		lcd.setCursor(15, 0);
		lcd.write(std::byte(1));
	}
}


static int evaluateButton(int x)
{
	int result = 0;


	if (x < 50) {
	    result = 1;
	} else if (x < 195) {
	    result = 2;
	}


	return result;
}


static void operateMainMenu(int *menuPage, int maxMenuPages, std::string *menuItems)
{
	int readKey, activeButton = 0;
	while (activeButton == 0) {
		int button;
		readKey = analogRead(0);
		button = evaluateButton(readKey);
		switch(button) {
		case 0:
		    break;
		case 1:
		    button = 0;
		    *menuPage = *menuPage - 1;
		    *menuPage = constrain(*menuPage, 0, maxMenuPages);
		    mainMenuDraw(menuItems, *menuPage, maxMenuPages);
		    activeButton = 1;
		    break;
		 case 2:
		    button = 0;
		    *menuPage = *menuPage + 1;
		    *menuPage = constrain(*menuPage, 0, maxMenuPages);
		    mainMenuDraw(menuItems, *menuPage, maxMenuPages);
		    activeButton = 1;
		    break;
		}
	}
}


void setup() {
    	// TODO
	std::byte downArrow[8] = { (std::byte)0b00100, (std::byte)0b00100,
                                   (std::byte)0b00100, (std::byte)0b00100,
                                   (std::byte)0b00100, (std::byte)0b11111,
                                   (std::byte)0b01110, (std::byte)0b00100 };
	std::byte upArrow[8]   = { (std::byte)0b00100, (std::byte)0b01110,
                                   (std::byte)0b11111, (std::byte)0b00100,
                                   (std::byte)0b00100, (std::byte)0b00100,
                                   (std::byte)0b00100, (std::byte)0b00100 };
	lcd.begin(16, 2);
	lcd.createChar(std::byte(1), upArrow);
	lcd.createChar(std::byte(2), downArrow);
}
void loop() {
    // TODO


    std::string menuItems[] = {"START CAPTURE", "START SHOWCASE" ,"PRESETS",
                           "SET TRIGGER", "SETTINGS", "ABOUT"};
    int maxMenuPages = round(((sizeof(menuItems) / sizeof(std::string)) / 2) + .5);


    mainMenuDraw(menuItems, menuPage, maxMenuPages);
    operateMainMenu(&menuPage, maxMenuPages, menuItems);
}


int main () {
    setup();
    while (true) {
        loop();
    }
}

I however have a feeling that this could be refactored even more. Could someone please give me a review? Make suggestions etc?

Any help would be much appreciated.

Thanks in advance!

[Salem]

Create enums for
- the key codes
- the special glyphs

Eg.

enum keyType {
  KEY_NONE = 0,
  KEY_UP = 1,
  KEY_DOWN = 2,
};
enum charType {
  CHAR_UP_ARROW = 1,
  CHAR_DOWN_ARROW = 2,
};

static keyType evaluateButton(int x) {
    keyType result = KEY_NONE;
    if (x < 50) {
        result = KEY_UP;
    } else if (x < 195) {
        result = KEY_DOWN;
    }
    return result;
}

// make things const where they can be
// use bool for 0|1 flags
// declare variables at the point of first use
// If this really is C++, then use a reference for menuPage, not a pointer.
static void operateMainMenu(int *menuPage, const int maxMenuPages, const std::string *menuItems)
{
    bool activeButton = true;
    while (activeButton) {
        int readKey = analogRead(0);
        keyType button = evaluateButton(readKey);
        switch(button) {
        case KEY_NONE:
            activeButton = false;
            break;
        case KEY_UP:
            *menuPage = *menuPage - 1;
            *menuPage = constrain(*menuPage, 0, maxMenuPages);
            mainMenuDraw(menuItems, *menuPage, maxMenuPages);
            break;
         case KEY_DOWN:
            *menuPage = *menuPage + 1;
            *menuPage = constrain(*menuPage, 0, maxMenuPages);
            mainMenuDraw(menuItems, *menuPage, maxMenuPages);
            break;
        }
    }
}

// It takes up more lines, but it's instantly obvious
// what the glyph really looks like.
void setup() {
    static const std::byte downArrow[8] = {
        (std::byte)0b00100, 
        (std::byte)0b00100,
        (std::byte)0b00100, 
        (std::byte)0b00100,
        (std::byte)0b00100, 
        (std::byte)0b11111,
        (std::byte)0b01110, 
        (std::byte)0b00100 
    };
    static const std::byte upArrow[8] = { 
        (std::byte)0b00100, 
        (std::byte)0b01110,
        (std::byte)0b11111, 
        (std::byte)0b00100,
        (std::byte)0b00100, 
        (std::byte)0b00100,
        (std::byte)0b00100, 
        (std::byte)0b00100 
    };
    lcd.begin(16, 2);
    lcd.createChar(std::byte(CHAR_UP_ARROW), upArrow);
    lcd.createChar(std::byte(CHAR_DOWN_ARROW), downArrow);
}

> int maxMenuPages = round(((sizeof(menuItems) / sizeof(std::string)) / 2) + .5);
It would seem that the 2 is somehow related to the number of lines on the display.

You might want to think about how this code would change if you upgraded to a 4-line display.
mainMenuDraw() should have some parameters and perhaps a loop.

Try and remove the global variable.

[thmm]

I would get rid of this C-style arrays and use std::array.
You also could use std::string_view instead of std::string.

[ghoul]

    static const std::byte downArrow[8] = {
        (std::byte)0b00100, 
        (std::byte)0b00100,
        (std::byte)0b00100, 
        (std::byte)0b00100,
        (std::byte)0b00100, 
        (std::byte)0b11111,
        (std::byte)0b01110, 
        (std::byte)0b00100 
    };
    static const std::byte upArrow[8] = { 
        (std::byte)0b00100, 
        (std::byte)0b01110,
        (std::byte)0b11111, 
        (std::byte)0b00100,
        (std::byte)0b00100, 
        (std::byte)0b00100,
        (std::byte)0b00100, 
        (std::byte)0b00100 
    };

I agree with the use 'const' here but why did you use the storage specifier 'static' in this context? Is it really necessary? I don't understand...

[Salem]

Well static will mean the array isn't constructed at run-time on the stack.