C++ CLI Console Menu Statemachine

Question

I am trying to find the best way(/most efficent way) to setup my console program's menu. The way I have it set up is that I have a Menu class and then I use a menu pointer called currentMenu and I just change where it is pointing to. it runs fairly well so far but I'd like to know what I could do to improve it

So Far it is seperated into 3 files(main.cpp,menu.h and menu.cpp) Full Code is here: https://repl.it/@SamuelHabtu/StudyBuddy-CLI

Menu.h:

#ifndef MENU_H
#define MENU_H
#include <iostream>
#include <string>
class menu 
{
public:
  std::string name;
  menu *prev;
  menu *right;
  menu *down;
  menu *up;
  void displayMenu();
};
#endif

Menu.cpp:

#include "menu.h"
void menu::displayMenu()
{
  std::string userInput;
  std::cout<<name<<" menu:\n";
  if(prev != 0)
  {
    std::cout<<"0:"<<prev->name<<" menu\n";
  }
  if(up != 0)
  {
    std::cout<<"1:"<<up->name<<" menu\n";  
  }
  if(down != 0)
  {
    std::cout<<"2:"<<down->name<<" menu\n";
  }
  if(right != 0)
  {
    std::cout<<"3:"<<right->name<<" menu\n";  
  }
}

Here is the function that I use to "fill" each menu

void buildMenu(menu *menu,std::string name,class menu *prev,class menu* up,class menu* down,class menu* right)
{
  menu->name = name;
  menu->prev = prev;
  menu->right = right;
  menu->down = down;
  menu->up = up;
}

This is How I am handling the Userinputs:

bool valid(std::string userInput,menu *currentMenu)
{
  if(userInput != "1" && userInput != "2" && userInput != "3" && userInput != "0")
  {
    std::cout<<"invalid input \n";
    return false;
  }
  if(userInput == "0" && currentMenu->prev == 0)
  {
    std::cout<<"invalid input \n";
    return false;    
  }
  if(userInput == "1" && currentMenu->up == 0)
  {
    std::cout<<"invalid input \n";
    return false;    
  }
  if(userInput == "2" && currentMenu->down == 0)
  {
    std::cout<<"invalid input \n";
    return false;    
  }
  if(userInput == "3" && currentMenu->right == 0)
  {
    std::cout<<"invalid input \n";
    return false;    
  }
  return true;
}

mainloop for the menu statemachine:

  while(go)
  {
    currentMenu->displayMenu();
    std::cin>>userInput;
    while(!valid(userInput,currentMenu)|| (userInput.size() > 1)) 
    {
      std::cin.clear();
      std::cin.ignore(std::numeric_limits<std::streamsize>::max(),'\n');
      std::cin>>userInput;
    }
    switch(userInput[0])
    {
    case '0':
      currentMenu = currentMenu->prev;
      break;
    case '1':
      currentMenu = currentMenu->up;
      break;
    case '2':
      currentMenu = currentMenu->down;
      break;
    case '3':
      currentMenu = currentMenu->right;
    }
  }

Answer 1

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", &notesMenu);
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(&currentMenu->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