Use a constructor to initialize class variables
Your function buildMenu()
looks like it is just initializing all the members of a class menu
. That is normally the task of a constructor. So why not write:
class menu
{
public:
...
menu(std::string name, menu *prev, menu* up, menu* down, menu* right):
name(name), prev(prev), right(right), down(down), up(up) {}
};
Note that since this is a simple POD class, you can also use brace initialization without having to write a constructor yourself, like so:
menu m = {"foo", nullptr, nullptr, nullptr, nullptr};
However, there is an even better way:
You only need to link menu
items to their parent
Manually specifying all four pointers is cumbersome, and it is easy to confuse the four directions. It is also not necessary to store four pointers, there is quite a bit of redundancy in them. It is better to write a constructor that just takes one pointer called parent
, and have the class menu
have a vector of pointers to child menus, like so:
class menu {
std::vector<menu *> children;
public:
const string name;
menu(const std::string &name, menu *parent = nullptr): name(name) {
if (parent) {
parent->children.push_back(this);
}
}
menu *getChild(int i) {
return children[i];
}
};
This makes constructing a menu hierarchy much easier:
menu mainMenu("Main");
menu toolMenu("Tools", &mainMenu);
menu notesMenu("Notes", &mainMenu);
menu subjectsMenu("Subjects", ¬esMenu);
menu mathMenu("Math", &subjectsMenu);
menu sciencesMenu("Sciences", &subjectsMenu);
menu humanMenu("Humanities", &subjectsMenu);
menu flashCardsMenu("Flash Cards", &mainMenu);
Of course, now navigating the menu becomes more tricky, but you should just need two variables:
std::stack<menu *> menuStack;
int menuItem;
The menuStack
is to keep track of which (sub)menu you are in, and menuItem
tells you which item in the current menu is selected. Navigating them becomes a matter of incrementing/decrementing menuItem to go up and down the currently selected menu, and pushing/popping menus to the stack to go deeper or going out of menus.
(Instead of storing a vector of children, you can still use four pointers. Your constructor still only needs to take a pointer to the parent menu, but now you need to spend more effort in the constructor to derive the four direction pointers based on the parent pointer.)
Further improvements
With the above, you still need to manually specify parent pointers.
You could instead add a member function to the class that creates a submenu:
class menu {
...
private:
menu(const std::string &name, menu *parent): name(name) {
parent->children.push_back(this);
}
public:
menu(const std::string &name): name(name) {}
menu subMenu(const std::string &name) {
return menu(name, this);
}
};
This way, you can create your menu hierarchy like this:
menu mainMenu("Main");
menu toolMenu = mainMenu.subMenu("Tools");
menu notesMenu = mainMenu.subMenu("Notes");
menu subjectsMenu = notesMenu.subMenu("Subjects");
menu mathMenu = subjectsMenu.subMenu("Math");
menu sciencesMenu = subjectsMenu.subMenu("Sciences");
menu humanMenu = subjectsMenu.subMenu("Humanities");
menu flashCardsMenu = mainMenu.subMenu("Flash Cards");
Now your main()
function doesn't have to deal with pointers anymore.
Start thinking about ownership
Currently, you are creating a number of menu
variables, and they are linked together using pointers. Regardless of how nice you make the interface of linking them together, this can still create a problem if there is the possibility for some of the menu
variables to go out of scope, and you get dangling pointers. There are several solutions:
Create a destructor that removes itself from its parent's children
vector. To do this, you will also need to store a menu *parent
pointer.
Use std::shared_ptr<>
to manage links between menu items. This requires that you remove the public constructors, and add a static member function that will do new menu(...)
and returns the result as a shared_ptr<menu>
.
Store submenus directly inside a regular menu. For example:
class menu {
vector<menu> children;
...
public:
menu &subMenu(const std::string &name) {
return children.emplace_back(name);
}
};
Create a cursor class for navigating the menu
Think of a cursor as a more advanced iterator. In this case, it's an iterator that can move both up and down inside a menu, and left and right to different menus. It should look like:
class menuCursor {
menu &rootMenu;
...
public:
menuCursor(menu &rootMenu): rootMenu(rootMenu) {
...
}
bool prev();
bool next();
bool up();
bool down();
menu &operator *() {
// return a reference to currently selected item
}
menu *operator ->() {
// return a pointer to the currently selected item
}
};
The functions prev()
, next()
and so on handle navigating the menu. Each should also check wether the action is correct, and return an error if not (here I recommend using a bool
to signal success). Like a regular iterator, you can overload operator *
to return the current menu item the cursor points to. The implementation depends on how you implement your menu
class: if you have four pointers, then follow those. If you use my suggested method, then keep a stack and an int around to keep track of where you are. Then in your main()
, you can do:
menu mainMenu("Main");
menuCursor cursor(mainMenu);
...
while(...get input...) {
cursor->displayMenu();
switch(input) {
case '0':
cursor.prev();
break;
case '1':
cursor.up();
break;
...
}
}
Move displayMenu()
out of class menu
The function displayMenu()
is displaying the possible options for navigating the menu. It makes more sense to have it be a member function of class menuCursor
, since the latter should know more about which navigation options are valid.
Use const
where appropriate
When passing pointers or references to variables to a function that should not modify those variables, you should make those parameters const
. It is quite common to pass strings as const references, like so:
menu(const std::string &name, ...);
This avoids the caller having to make a copy of the string.
Class member variables that are only set at initialization time, and will never change, can also be made const. In your case, if you know you will never change the name of a menu entry after having created it, make it const:
class menu {
...
const std::string name;
...
};
Consider a different way of navigating the menus
Instead of offering four options (prev, next, up, down), you could instead show all possible options in the current menu, plus an option to exit the current (sub)menu. Something like:
std::stack<menu *> menuStack;
menuStack.push(&mainMenu);
while (!menuStack.empty()) {
menu *currentMenu = menuStack.top();
// Print current menu
std::cout << currentMenu->name << ":\n";
int i = 0;
for (auto &submenu: currentMenu->children) {
std::cout << i++ << ". " << submenu.name << "\n";
}
std::cout << "X. Exit current menu\n";
// Get input
std::string input;
std::cin >> input;
// Navigate based on the input
if (input == "X") {
menuStack.pop();
continue;
} else {
int item = std::stoi(input);
// Add error checking here to ensure item is valid
menuStack.push(¤tMenu->children[item]);
}
}
This should show you something like:
Main:
0. Tools
1. Notes
2. Subjects
X. Exit current menu
And when you enter 2, it should show:
Subjects:
0. Math
1. Sciences
2. Humanities
X. Exit current menu
main.cpp
here to give reviewers a better idea of how you intend to use this? \$\endgroup\$