Unrendering a Component of an embedded C++ Graphics Library

Question

This is an extra short one, since I need some guidance on best practice for C++. It's been a while since I did some C++ in general, and its the first time for me developing on a microcontroller.

I am currently creating a framework for displaying stuff on a small TFT-Screen. I have some abstract classes to make life easier, one of the methods will unrender the component that I am considering. I wrote the code about half a year ago and am doing some refactoring. I don't know anymore, why I wrote the code like that, I found it in the codebase without any hint in the git history.

void SGKeyboard::unrender(Position position)
{
 Position baseDrawPosition = position;
 baseDrawPosition.addX(getLeftSpacing());
 baseDrawPosition.addY(getTopSpacing());
 fillRect(baseDrawPosition.getX(), baseDrawPosition.getY(), width, height, SGTheme::getTheme()->backgroundColor);
}

My understanding of C++ clearly tells me, the baseDrawPosition object is not necessary here. I could directly work with the position object, because it is copied when passed to the unrender function.

I've had disagreements with coworkers about if it would be good or bad style to remove this variable. The most common argument to keep it would be code clarity, which I don't really see. After all, I just introduce another variable, that really serves no purpose. But I am open to explanations why this is better than just working on the position object. A fresh unbiased perspective would be greatly appreciated here.

Here is the class definition of the keyboard, and also the definition of the position:

 /**
 * The SGKeyboard class represents a keyboard component in the SmallGUI library.
 * The keyboard will probably take up most of the screen space, so using multiple keyboards on the same screen is not recommended.
 * 
 * | 1 2 3 4 5 6 7 8 9 0 <- |
 * | Q W E R T Y U I O P <- |
 * | A S D F G H J K L ; ENTER |
 * | SHIFT Z X C V B N M . < ENTER |
 * | ----SPACEBAR----- RESET |
 * 
 * It inherits from SGComponent, SGClickable, SGTexted, and SGTargetting classes.
 * The SGKeyboard class provides functionality for rendering and interacting with a keyboard.
 */
 class SGKeyboard : public SGComponent, public SGClickable, public SGTexted, public SGTargetting
 {
 public:
 /**
 * @brief Destructor for the SGKeyboard class.
 */
 ~SGKeyboard();
 /**
 * @brief Renders the keyboard component at the specified position.
 * 
 * @param position The position at which the keyboard should be rendered.
 */
 void render(SmallGUI::Position position) override;
 /**
 * @brief Unrenders the keyboard component at the specified position.
 * 
 * @param position The position at which the keyboard should be unrendered.
 */
 void unrender(SmallGUI::Position position) override;
 /**
 * @brief Gets the total width of the keyboard component including spacing.
 * 
 * @return The total width of the keyboard component.
 */
 int getTotalXSize() override;
 /**
 * @brief Gets the total height of the keyboard component including spacing.
 * 
 * @return The total height of the keyboard component.
 */
 int getTotalYSize() override;
 /**
 * @brief Sets the visibility of the keyboard component.
 * 
 * @param visible The visibility state to set.
 */
 void setVisible(bool visible) override;
 /**
 * @brief Checks if the keyboard component is visible.
 * 
 * @return True if the keyboard component is visible, false otherwise.
 */
 bool isVisible() const override;
 /**
 * @brief Handles the activation of the keyboard component at the specified position.
 * 
 * @param position The position at which the keyboard was activated.
 */
 void activated(SmallGUI::Position position) override;
 /**
 * @brief Sets the target value for the keyboard component.
 * 
 * @param targetValue A pointer to the target value to be set.
 */
 void setTargetValue(String* targetValue);
 /**
 * @brief Sets the action to be performed when the enter button is pressed.
 * 
 * @param enterAction A pointer to the SGAction to be set.
 */
 void setEnterAction(SGAction* enterAction);
 private:
 /// @brief The action to execute when the enter button is pressed.
 SGAction* enterAction = nullptr;
 /// @brief The target value of the keyboard component.
 String* targetValue;
 /// @brief The visibility of the keyboard component.
 bool visible = false;
 /// @brief Is the keyboard in caps mode?
 bool caps = false;
 /// @brief The unit width of a button.
 int buttonWidth = 44;
 /// @brief The unit height of a button.
 int buttonHeight = 44;
 /**
 * @brief Renders the bottom bar of the keyboard component.
 */
 void renderBottomBar();
 /**
 * @brief Renders the side bar of the keyboard component.
 */
 void renderSideBar();
 /**
 * @brief Renders the main area of the keyboard component.
 */
 void renderMainArea();
 /**
 * @brief Renders the numbers section of the keyboard component.
 */
 void renderNumbers();
 /**
 * @brief Renders the top letters section of the keyboard component.
 */
 void renderTopLetters();
 /**
 * @brief Renders the center letters section of the keyboard component.
 */
 void renderCenterLetters();
 /**
 * @brief Renders the bottom letters section of the keyboard component.
 */
 void renderBottomLetters();
 /**
 * @brief Renders the space bar button of the keyboard component.
 * 
 * @param highlighted Indicates whether the button should be highlighted.
 */
 void renderSpaceBar(bool highlighted);
 /**
 * @brief Renders the reset button of the keyboard component.
 * 
 * @param highlighted Indicates whether the button should be highlighted.
 */
 void renderResetButton(bool highlighted);
 /**
 * @brief Renders the remove button of the keyboard component.
 * 
 * @param highlighted Indicates whether the button should be highlighted.
 */
 void renderRemoveButton(bool highlighted);
 /**
 * @brief Renders the enter button of the keyboard component.
 * 
 * @param highlighted Indicates whether the button should be highlighted.
 */
 void renderEnterButton(bool highlighted);
 /**
 * @brief Renders a default button with the specified text at the specified position.
 * 
 * @param text The text to be displayed on the button.
 * @param position The position at which the button should be rendered.
 * @param highlighted Indicates whether the button should be highlighted.
 */
 void renderDefaultButton(char text, SmallGUI::Position position, bool highlighted);
 /**
 * @brief Gets the character corresponding to the clicked position on the keyboard component.
 * 
 * @param clickedPosition The position that was clicked.
 * @return The character corresponding to the clicked position.
 */
 char getClickedChar(SmallGUI::Position clickedPosition);
 /**
 * @brief Updates the letters section of the keyboard component.
 */
 void fastUpdateLetters();
 /**
 * @brief Updates the top letters section of the keyboard component.
 */
 void fastUpdateTopLetters();
 /**
 * @brief Updates the center letters section of the keyboard component.
 */
 void fastUpdateCenterLetters();
 /**
 * @brief Updates the bottom letters section of the keyboard component.
 */
 void fastUpdateBottomLetters();
 /**
 * @brief Updates a default button with a new character at the specified position.
 * 
 * @param old The old character on the button.
 * @param newChar The new character to be displayed on the button.
 * @param position The position of the button to be updated.
 * @param highlighted Indicates whether the button should be highlighted.
 */
 void fastUpdateDefaultButton(char old, char newChar, SmallGUI::Position position, bool highlighted);
 };

Position:

class Position
 {
 public:
 Position(int, int);
 Position();
 int getX();
 int getY();
 void setX(int);
 void setY(int);
 void addX(int);
 void addY(int);
 Position incrementedX(int);
 Position incrementedY(int);
 Position operator*(int multiplier);
 Position operator/(int divisor);
 Position operator+(const Position &other);
 Position operator-(const Position &other);
 private:
 int x, y;
 };

Position::Position(int x, int y) {
 this->x = x;
 this->y = y;
}
Position::Position() {
 this->x = 0;
 this->y = 0;
}

• Fe2O3: Can you edit the question to add the class definition, or at least the applicable constructor? It may be just vestigial from some earlier version; it may be a novice who was unsure about what to do; or it may be important that a discrete object is created for temporary use. Need more info... (Admonishment: Always good to explain why with a comment. Takes a few seconds, and saves hours of head-scratching.)
• monamona: Added the headers of both keyboard and position :)
• Fe2O3: Thanks for adding those. :-) Superficially, it appears the seeming extra copy is not needed and your theory is right. Disturbing (to me) is the use of the "simple" Position class. Do its (x,y) refer to screen pixel offsets (from topLeft or bottomLeft?), or are they keyboard character "grid coordinates"? (Eg: 'S' is in row 3, column 2 (or row 2, col 1, base 0). Appreciate the trouble with representation, but it appears some keys may be wider or taller than others. One width or height won't serve all keys on the keyboard. Maxim: "Suck it & see" (if it breaks.) Less code IS better code.
• monamona: @Fe2O3 The width is the total width of the keyboard. The keys themselves are all dimensioned by int factors of buttonWidth and buttonHeight. The Position is the pixel offset to the top left corner of the screen.
• Fe2O3: @monamona It appears this "review" post didn't meet with approval... It's only my opinion: The dubious copy object appears to be unnecessary, and its presence makes you (and the next reader) wonder, "Why is that there?" Recommend you make a duplicate of the function, comment-out one preserved version, then make the change you now believe would eliminate that extra object. Then, test, test, test and test again. Less code is better code. (Tip: use #if 0/#else/#endif to retain two versions while only one is active...) Best wishes! :-)

Answer 1

Simplify Position

It's great that you use a class to group x and y coordinates and allow operations on the whole position instead of only on the individual components. However, making x and y private and only allowing access via getters and setters is not improving anything, it just makes it cumbersome to use this class. I recommend rewriting it like so:

struct Position {
 int x = 0;
 int y = 0;
 Position operator*(int multiplier);
 Position operator/(int divisor);
 Position operator+(const Position &other);
 Position operator-(const Position &other);
}

Note the absence of constructors and copy/move assignment operators; these are all taken care of by the compiler automatically.

Alternatively, use an external library that provides vector types, such as Eigen or GLM. Or just have a look at them to see what their approach is.

Make better use of Position

Try to use Position for all things where you need to pass two-dimensional coordinates. Consider being able to write:

void SGKeyboard::unrender(Position position)
{
 Position baseDrawPosition = position + getSpacing();
 fillRect(baseDrawPosition, width, height, SGTheme::getTheme()->backgroundColor);
}

Where getSpacing() returns a Position containing the left and top spacing. Of course, you might say, that's not a position anymore. In that case, consider renaming Position to Vec2d or something like that.

Uninitialized member variable

I don't see a constructor in SGKeyboard, and thus the pointer targetValue can thus easily be left uninitialized. If this pointer is then dereferenced, then in the best case your program crashes, in the worst case it doesn't and will do something unexpected.

About modifying function parameters

You are right that baseDrawPosition isn't necessary for the functionality of your code. However, consider that the value of baseDrawPosition will be different from position when you are calling fillRect(). That means the two variables are pointing to different things. Having two different names for them is then also avoiding possible confusion.

Of course, it helps if the names where a bit more specific. What is position actually pointing to? The center of keyboard component? The top-left corner? It's hard to tell for me as a reviewer. Perhaps if a very consistent meaning is associated with the name position in your codebase, it is fine, but otherwise it might benefit from being renamed.

• Fe2O3: Been quite a few years since I used C++. OP is perplexed about local instance baseDraw... as parameter position is (should be) a local working copy. Curious... Would position += getSpacing(); be enough? Thanks :-)
• Fe2O3: Or, if I understand OP's description, perhaps each key's class contains, in addition to btnHeight & btnWidth, a one-time-only initialisation of its topLeftRow and topLeftCol values... A bit more memory, but buttons aren't expected to be dancing around the screen... short instead of int sizes... Maybe even unsigned char would have sufficient range for all pixels?!?! :-)