Thread: Code refactor

  1. #1
    Registered User
    Join Date
    Oct 2019
    Posts
    71

    Code refactor

    Hello friends,

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

    Code:
    #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:

    Code:
    #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!

  2. #2
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    39,073
    Create enums for
    - the key codes
    - the special glyphs

    Eg.
    Code:
    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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  3. #3
    Registered User
    Join Date
    Sep 2020
    Posts
    134
    I would get rid of this C-style arrays and use std::array.
    You also could use std::string_view instead of std::string.

  4. #4
    Registered User
    Join Date
    Oct 2019
    Posts
    71
    Quote Originally Posted by Salem View Post

    Code:
        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...

  5. #5
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    39,073
    Well static will mean the array isn't constructed at run-time on the stack.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

  6. #6
    Registered User
    Join Date
    Jan 2022
    Posts
    1
    This topic helped me too. I didn't thought about key codes





    Radaryonline.pl

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Replies: 5
    Last Post: 01-25-2021, 06:27 AM
  2. Replies: 2
    Last Post: 09-19-2012, 01:58 PM
  3. Replies: 1
    Last Post: 03-10-2010, 11:28 AM
  4. Replies: 14
    Last Post: 04-01-2008, 02:23 AM
  5. producing c/c++ code from flowcharts,pseudo code , algorithims
    By rohit83.ken in forum C++ Programming
    Replies: 3
    Last Post: 02-20-2008, 07:09 AM

Tags for this Thread