0
\$\begingroup\$

I have made a menu that utilizes the composite design pattern and would like to get some feedback on the code, so be as harsh as you can.

Menu.h:

#ifndef MENU_H
#define MENU_H

#include "Console.h"
#include <functional>
#include <iostream>
#include <memory>
#include <vector>

class ContainerComponent
{
public:
    virtual const std::string& getName() const = 0;
    virtual void call() = 0;

    virtual ~ContainerComponent() = default;
};

class CompositeMenu final : public ContainerComponent
{
public:
    void addChild(const std::unique_ptr<ContainerComponent> child);
    void removeChild(const std::unique_ptr<ContainerComponent> child);

    void addHeader(const std::string& header);
    void enableBoxedOptions(int boxHight = 3); 
    void disableBoxedOptions();
    void setMenuXY(int x, int y);

    virtual const std::string& getName() const override;
    virtual void call() override;

    explicit CompositeMenu(int x = 0, int y = 0, bool enabledBoxedMenu = false, int boxHight = 3)
        : m_menuXPos{ x }, m_menuYPos{ y }, m_enabledBoxedMenu{ enabledBoxedMenu }
    {//boxHight = 3 is the minimal size for symmetry
        if (m_enabledBoxedMenu)
        {
            enableBoxedOptions(boxHight);
        }//end of if
    }

private:
    std::vector<std::unique_ptr<ContainerComponent> > m_children;
    std::string                      m_header{};
    bool                             m_enabledBoxedMenu;
    bool                             m_enabledHeader{ false };
    int                              m_menuXPos;
    int                              m_menuYPos;
    unsigned int                     m_menuHight;
    unsigned int                     m_menuLength;
    unsigned int                     m_jumpRange{ 1 };

    void updateLengths(const std::string& opt, int padding);
    void printBoxedOptions(int& curXPos, int& curYPos) const;
    void printBareOptions(int& curXPos, int& curYPos) const;
    void printHeader(int& curXPos, int& curYPos) const;
    void printMenu(int curXPos, int curYPos) const;
    void executeOption(intPair coords) const;
    void updateMenuHight();
    int getPadding() const;
};

class Action final : public ContainerComponent
{
public:
    virtual const std::string& getName() const override;
    virtual void call() override;

    explicit Action(const std::string& actionName,
                    const std::function<void()> action)
        : m_actionName{ actionName }, m_action{ action } 
    {
    }

private:
    const std::function<void()> m_action;
    const std::string           m_actionName;
};

#endif

Menu.cpp:

#include "Menu.h"
#include <string>
#include <algorithm>

namespace
{
    unsigned constexpr char selectionArrow    { 175 };
    constexpr int           boxedLengthPadding{ 4 };
    constexpr int           boxedHightPadding { 2 };
    constexpr int           yRangePadding     { 2 };
    constexpr int           bareLengthPadding { 1 };
    constexpr int           xJumpRange        { 0 };

    int findMiddle(int num)
    {
        return ((num / 2) + 1);
    }
}

/*|---COMPOSITE_MENU_FUNCTIONS_START---|*/
/*|---PUBLIC_FUNCTIONS_START---|*/
void CompositeMenu::addChild(const std::unique_ptr<ContainerComponent> child)
{
    if (std::find_if(m_children.begin(),
                     m_children.end(),
                     [&child](const std::unique_ptr<ContainerComponent> cmp)
                     {return cmp->getName() == child->getName(); }) == m_children.end()) 
    {
        m_children.emplace_back(child); 
        if (m_enabledBoxedMenu) updateLengths(child->getName(), boxedLengthPadding);
        else                    updateLengths(child->getName(), bareLengthPadding);
    }//end of if
    else std::cout << "An Action with this name already exists.\n";
}

void CompositeMenu::removeChild(const std::unique_ptr<ContainerComponent> child)
{
    if (std::find_if(m_children.begin(),
                     m_children.end(),
                     [&child](const std::unique_ptr<ContainerComponent> cmp)
                     {return cmp.get() == child.get(); }) != m_children.end())
    {
        m_children.erase(std::remove_if(m_children.begin(),
                                        m_children.end(),
                                        [&child](const std::unique_ptr<ContainerComponent> cmp)
                                        {return cmp.get() == child.get(); }),
                                        m_children.end());
    }//end of if
    else std::cout << "The option doesn't exist.\n";
}

void CompositeMenu::addHeader(const std::string& header)
{
    m_enabledHeader = true;
    m_header        = header;
    if (static_cast<unsigned int>(m_header.length()) + 1 > m_menuLength)
    {
        m_menuLength = static_cast<unsigned int>(m_header.length()) + 1;
    }//end of if
}

void CompositeMenu::enableBoxedOptions(int boxHight)
{//must be an odd number for symmetry reasons
    if ((boxHight >= 3) && (boxHight % 2 != 0))
    {
        m_enabledBoxedMenu = true;
        m_jumpRange        = boxHight;
    }//end of if
    else std::cout << "Invalid box size.\n";
}

void CompositeMenu::disableBoxedOptions()
{
    if (m_enabledBoxedMenu)
    {
        m_enabledBoxedMenu = false;
        m_jumpRange = 1; //default jump range with no boxes
        updateMenuHight();
    }//end of if
}

void CompositeMenu::setMenuXY(int x, int y)
{
    Console::setXY(x, y, m_menuXPos, m_menuYPos);
}

/*|---VIRTUAL_FUNCTIONS_START---|*/
const std::string& CompositeMenu::getName() const
{
    return m_header;
}

void CompositeMenu::call() 
{
    if (static_cast<int>(m_children.size()) == 0)
        std::cout << "No options found, unable to print menu.\n";
    else
    {
        updateMenuHight();
        printMenu(m_menuXPos, m_menuYPos);
        executeOption(Console::navigation(intPair(xJumpRange, m_jumpRange),
                                          intPair(m_menuXPos + 1, m_menuXPos + 1),
                                          intPair(m_menuYPos + getPadding(),
                                          (m_menuYPos + getPadding()) + m_jumpRange * (static_cast<int>(m_children.size()) - 1)),
                                          selectionArrow));
    }//end of else
}
/*|----VIRTUAL_FUNCTIONS_END----|*/
/*|----PUBLIC_FUNCTIONS_END----|*/

/*|---PRIVATE_FUNCTIONS_START---|*/
void CompositeMenu::updateLengths(const std::string& opt, int padding)
{
    if ((static_cast<unsigned int>(opt.length()) + padding) > m_menuLength)
    {
        m_menuLength = static_cast<unsigned int>(opt.length()) + padding;
    }//end of if
}

void CompositeMenu::printBoxedOptions(int& curXPos, int& curYPos) const
{
    for (size_t index{}; index < m_children.size(); ++index)
    {
        Console::drawBox(m_jumpRange - boxedHightPadding, m_menuLength - boxedLengthPadding, curXPos, curYPos);
        Console::gotoxy(curXPos + 2, curYPos + findMiddle(m_jumpRange) - 1);
        std::cout << m_children[index]->getName();
        curYPos += m_jumpRange;
    }//end of for
}

void CompositeMenu::printBareOptions(int& curXPos, int& curYPos) const
{
    ++curXPos;
    for (size_t index{}; index < m_children.size(); ++index)
    {
        Console::gotoxy(curXPos, curYPos++);
        std::cout << m_children[index]->getName();
    }//end of for
}

void CompositeMenu::printHeader(int& curXPos, int& curYPos) const
{
    Console::gotoxy(curXPos + 1, curYPos);
    std::cout << m_header;
    Console::gotoxy(curXPos + 1, ++curYPos);
    for (size_t index{}; index < m_header.length(); ++index) std::cout << '-';
    ++curYPos;
}

void CompositeMenu::printMenu(int curXPos, int curYPos) const
{
    Console::drawBox(m_menuHight, m_menuLength, curXPos, curYPos);
    curXPos += 2;
    ++curYPos;
    if (m_enabledHeader)        printHeader(curXPos, curYPos);
    if (m_enabledBoxedMenu)     printBoxedOptions(curXPos, curYPos);
    else                        printBareOptions(curXPos, curYPos);
}

void CompositeMenu::executeOption(intPair coords) const
{
    int destX, destY;
    if (m_enabledHeader) destX = m_menuXPos + m_menuLength - 1;
    else                 destX = m_menuXPos + m_menuLength;
    if (m_enabledHeader) destY = m_menuYPos + m_menuHight - 3;
    else                 destY = m_menuYPos + m_menuHight;
    Console::clrSec(m_menuXPos, m_menuYPos, destX, destY);
    m_children[(coords.second - (m_menuYPos + getPadding())) / m_jumpRange]->call();
}

void CompositeMenu::updateMenuHight()
{
    if (m_enabledHeader) m_menuHight = m_jumpRange * static_cast<unsigned int>(m_children.size()) + 2;
    else                 m_menuHight = m_jumpRange * static_cast<unsigned int>(m_children.size());
}

int CompositeMenu::getPadding() const 
{
    if (m_enabledHeader) return(findMiddle(m_jumpRange) + yRangePadding);
    else                 return(findMiddle(m_jumpRange));
}
/*|----PRIVATE_FUNCTIONS_END----|*/
/*|----COMPOSITE_MENU_FUNCTIONS_END----|*/

/*|---ACTION_FUNCTIONS_START---|*/
/*|---PUBLIC_FUNCTIONS_START---|*/
/*|---VIRTUAL_FUNCTIONS_START---|*/
const std::string& Action::getName() const
{
    return m_actionName;
}

void Action::call() 
{
    m_action();
}
/*|----VIRTUAL_FUNCTIONS_END----|*/
/*|----PUBLIC_FUNCTIONS_END----|*/
/*|----ACTION_FUNCTIONS_END----|*/

I will also add the Console.h library that I wrote if anyone wants to use it:

Console.h:

#ifndef CONSOLE_GRAPHICS_H
#define CONSOLE_GRAPHICS_H

#include <utility>

using intPair = std::pair<int, int>;

namespace Console
{
    intPair navigation(const intPair& jumpLength,
        const intPair& xRange,
        const intPair& yRange,
        unsigned const char symbol);
    void setXY(int destX, int destY, int &curX, int &curY);
    void putSymbol(int xPos, int yPos, const char symbol);
    void clrSec(int curX, int curY, int destX, int destY);
    void drawBox(int hight, int length, int x, int y);
    void gotoxy(int x, int y);
    void clrScr();
}

#endif

Console.cpp:

#include "Constants.h"
#include "Console.h" 
#include <iostream>
#include <Windows.h>
#include <conio.h>

namespace
{
    constexpr int           downKey { 80 };
    constexpr int           rightKey{ 77 };
    constexpr int           leftKey { 75 };
    constexpr int           upKey   { 72 };
    unsigned constexpr char enterKey{ 13 };
    constexpr int           padding { 2 };
}

intPair Console::navigation(const intPair& jumpLength,
    const intPair& xRange,
    const intPair& yRange,
    unsigned const char symbol)
{
    int curX{ xRange.first };
    int curY{ yRange.first };
    do {
        putSymbol(curX, curY, symbol);
        gotoxy(curX, curY);
        if (_getch() == enterKey) return intPair(curX, curY);
        switch (static_cast<int>(_getch()))
        {
        case upKey:
        {
            if ((curY - jumpLength.second) < yRange.first)
            {
                curY = yRange.second;
                putSymbol(curX, yRange.first, ' ');
            }//end of if
            else
            {
                curY -= jumpLength.second;
                putSymbol(curX, curY + jumpLength.second, ' ');
            }//end of else
        }//end of case 
        break;
        case downKey:
        {
            if ((curY + jumpLength.second) > yRange.second)
            {
                curY = yRange.first;
                putSymbol(curX, yRange.second, ' ');
            }//end of if
            else
            {
                curY += jumpLength.second;
                putSymbol(curX, curY - jumpLength.second, ' ');
            }//end of else
        }//end of case 
        break;
        case leftKey:
        {
            if ((curX - jumpLength.first) < xRange.first)
            {
                curX = xRange.second;
                putSymbol(xRange.first, curY, ' ');
            }//end of if
            else
            {
                curX -= jumpLength.first;
                putSymbol(curX + jumpLength.first, curY, ' ');
            }//end of else
        }//end of case 
        break;
        case rightKey:
        {
            if ((curX + jumpLength.first) > xRange.second)
            {
                curX = xRange.first;
                putSymbol(xRange.second, curY, ' ');
            }//end of if
            else
            {
                curX += jumpLength.first;
                putSymbol(curX - jumpLength.first, curY, ' ');
            }//end of else
        }//end of case 
        break;
        }//end of switch
    } while (true);
}

void Console::setXY(int destX, int destY, int &curX, int &curY)
{
    if (destX < 0)
    {
        curX = -destX;
        std::cout << "X value was made positive.\n";
    }//end of if
    else curX = destX;
    if (destY < 0)
    {
        curY = -destY;
        std::cout << "Y value was made positive.\n";
    }//end of if
    else curY = destY;
}

inline void Console::putSymbol(int xPos, int yPos, const char symbol)
{
    gotoxy(xPos, yPos);
    std::cout << symbol;
}

void Console::clrSec(int curX, int curY, int destX, int destY)
{
    gotoxy(curX, curY);
    for (int row{}; row < destY; ++row)
        for (int col{}; col < destX; ++col)
        {
            Console::gotoxy(curX + col, curY + row);
            std::cout << ' ';
        }//end of for
    gotoxy(curX, curY);
}

void Console::drawBox(int hight, int length, int x, int y)
{
    gotoxy(x, y);
    std::cout << Shapes::topLeftAngle;
    for (int i{}; i < (length + padding); ++i) std::cout << Shapes::horizontalPiece;
    std::cout << Shapes::topRightAngle;

    for (int i{}; i < hight; ++i)
    {
        gotoxy(x, ++y);
        std::cout << Shapes::verticalPiece;
        gotoxy(x + length + 3, y); // x + length + 3 represents the furthest boarder                           
        std::cout << Shapes::verticalPiece;
    }//end of for

    gotoxy(x, ++y);
    std::cout << Shapes::bottomLeftAngle;
    for (int i{}; i < (length + padding); ++i) std::cout << Shapes::horizontalPiece;
    std::cout << Shapes::bottomRightAngle;
}

inline void Console::gotoxy(int x, int y)
{
    COORD pos = { static_cast<short>(x), static_cast<short>(y) };
    SetConsoleCursorPosition(GetStdHandle(STD_OUTPUT_HANDLE), pos);
}

void Console::clrScr()
{
    CONSOLE_SCREEN_BUFFER_INFO csbi;
    HANDLE                     hStdOut;
    DWORD                      count;
    DWORD                      cellCount;
    COORD                      homeCoords = { 0, 0 };

    hStdOut = GetStdHandle(STD_OUTPUT_HANDLE);
    if (hStdOut == INVALID_HANDLE_VALUE) return;

    if (!GetConsoleScreenBufferInfo(hStdOut, &csbi)) return;
    cellCount = csbi.dwSize.X *csbi.dwSize.Y;

    if (!FillConsoleOutputCharacter(
        hStdOut,
        (TCHAR) ' ',
        cellCount,
        homeCoords,
        &count
    )) return;

    SetConsoleCursorPosition(hStdOut, homeCoords);
}

Constants.h:

#ifndef CONSTANTS_H
#define CONSTANTS_H

namespace Shapes
{
    extern unsigned const char bottomRightAngle;
    extern unsigned const char topRightAngle;
    extern unsigned const char bottomLeftAngle;
    extern unsigned const char topLeftAngle;
    extern unsigned const char horizontalPiece;
    extern unsigned const char verticalPiece;
}

#endif 

Constants.cpp:

#include "Constants.h"

namespace Shapes
{
    extern unsigned const char bottomRightAngle{ 188 };
    extern unsigned const char topRightAngle   { 187 };
    extern unsigned const char bottomLeftAngle { 200 };
    extern unsigned const char topLeftAngle    { 201 };
    extern unsigned const char horizontalPiece { 205 };
    extern unsigned const char verticalPiece   { 186 };
}

Note that the method used for Constants.h is in place because it is meant to be included in a bunch of files and I don't want the constants to be defined in each separate file all over again.

Lastly, here is the source.cpp I used for testing:

#include "Menu.h"
#include "Console.h"

void a() { std::cout << 'a'; }
void b() { std::cout << 'b'; }
void c() { std::cout << 'c'; }
void d() { std::cout << 'd'; }

int main()
{
    std::unique_ptr<CompositeMenu> menu{ std::make_unique<CompositeMenu>(5, 5, true) };
    std::unique_ptr<CompositeMenu> submenu{ std::make_unique<CompositeMenu>(5, 5) };
    submenu->addHeader("Sub menu");
    menu->addHeader("Main menu");
    std::unique_ptr<ContainerComponent> optA{ std::make_unique<Action>("a", a) };
    std::unique_ptr<ContainerComponent> optB{ std::make_unique<Action>("b", b) };
    std::unique_ptr<ContainerComponent> optC{ std::make_unique<Action>("c", c) };
    std::unique_ptr<ContainerComponent> optD{ std::make_unique<Action>("d", d) };
    menu->addChild(std::move(optA));
    menu->addChild(std::move(optB));
    menu->addChild(std::move(optC));
    menu->addChild(std::move(optD));
    menu->removeChild(std::move(optB));

    submenu->addChild(std::move(optA));
    submenu->addChild(std::move(optB));
    submenu->addChild(std::move(optC));
    submenu->addChild(std::move(optD));

    menu->addChild(std::move(submenu));
    menu->call();
    return 0;
}
\$\endgroup\$
2
  • \$\begingroup\$ Choose const unsigned char over unsigned const char. The const is likely to be overlooked \$\endgroup\$ Jul 2, 2017 at 13:17
  • \$\begingroup\$ Also, I don't think using intPair = std::pair<int, int>; actually helps readability. I would go without it. \$\endgroup\$ Jul 2, 2017 at 13:20

1 Answer 1

1
\$\begingroup\$

Just a few hints I gathered:

  1. const should be the first modifier in most cases. Things like unsigned const char are bad because most people are used to thinking of it as an unsigned char which is also const, in contrast to a const char which is also unsigned (i.e. the first level of categorization is usually unsigned, then const).
  2. Leave a space between #include and the corresponding angle brackets/quotation marks.
  3. The fact that Constants.h has a "meanings" comment hints you at the fact that you should probably choose better names for your variables. Usually, typing a few more characters is not going to hurt your coding performance very much (especially not in times of auto completion!) and will make you code much more readable. As a reviewer, I do not want to switch back to Constants.h to look up the meaning of a variable every time I encounter one.
  4. CompositeMenu::addChild should probably take its parameter by const reference, because you are not actually doing any modifications to the object.
  5. You should rethink your approach on ownership. Currently, CompositeMenu does not own its children, which means that they have to be owned by something else. That means that your whole design structure is not self-sufficient; you always need some kind of class that maintains a vector of all current ContainerComponents, to which each of your CompositeMenus only holds a reference. You also need to make sure that none of the objects expire before your CompositeMenu, or else you will run into undefined behavior. Also, if any of your ContainerComponents are moved, you will need to update their reference accordingly in their CompositeMenu, which is cumbersome at best and invokes undefined behavior at worst if you make a mistake. Thus, I suggest you to change the relationship into an owning one, and possible give out references or pointers to other users (also, you still need to face the issue of relocation and thus invalidation, which is why you will probably need another layer of indirection (i.e. a vector of ContainerComponent*))
  6. If you want to begin your class definitions with the public members, I would recommend you to use struct instead, since all members of struct are implicitly public unless marked otherwise (still, this is very much a question of personal style)
  7. Use the correct integer types. For example, it appears that m_menuLength can never be lower than 0, so it should at the very least be unsigned. However, for variables denoting a length, std::size_t is generally more appropriate (especially since you assign it the length of a string, which is also of type std::size_t).
\$\endgroup\$
1
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ Jul 3, 2017 at 13:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.