C++ Console Menu User Validation

Question

So I am trying to implement a menu in a program I am writing. My goal is to have zero errors when collecting user input. So I thought the best way to validate was to capture the input as a string and then test against it and prompt for input again until it is valid.

As far as I can tell, my code works as I want it to. BUT, is this the best way to go about this? Almost every example online uses char or int for the input, are there benefits to this?. I feel like I made it more complex than it needed to be. As a beginner, any suggestions would be appreciated.

#include <iostream>
#include <string>
using namespace std;


string GetInput()
{
    string s = "";

    cout << ">> ";
    getline(cin, s);

    return s;
}
void mainMenu() {

    cout << "Select an option" << endl;
    cout << "1. Add Entry" << endl;
    cout << "2. Edit Entry" << endl;
    cout << "Q. Quit" << endl;

    string input = GetInput();

    while (!(input.find_first_not_of("12qQ") == string::npos) || input.empty())
    {
        input = GetInput();
    }

    if (input == "1") {
        cout << "Add Entry" << endl;
        //AddEntry();
    }

    else if (input == "2") {
        cout << "Edit Entry" << endl;
    }

    else {
        cout << "Quit" << endl;
    }

}

int main() {

    cout << "===================================================" << endl;
    cout << "|                 Secure Database                 |" << endl;
    cout << "===================================================" << endl << endl;

    mainMenu();

    return 0;
}

Answer 1

Let me collect a couple of thoughts here.

• When the input validation is not successful, users immediately get the ">>" prompt without any notification that their input wasn't accepted. Maybe add a hint that no action was performed?

• Almost every example online uses char or int for the input, are there benefits to this?

  The input that you ask for so far is one out of 1, 2, q or Q, i.e., 4 entries from the ASCII table. Using a single char for this is a reasonable choice, but might be limiting when the program grows. Maybe you want to ask users for paths, tokens, anything with more than one character? Having setup everything with std::string in the first place might be the more pragmatic approach, as it simplifies things (e.g. one function for both the one-char-only and multi-char-input).

• In GetInput(), you are doing more work than necessary:

  string s = "";
  

  You want an empty string instance to be passed to std::getline, then default-construct it with std::string s; instead of initializing it with an empty string literal (remember that the type of "" is const char[1], std::string s = ""; is not a no-op), see also Item 4 in Scott Meyer's Effective C++.

• You flush the standard output a lot of time, when you actually just want a newline \n to be printed out. See this thread for more details.

• This is in my opinion the most severe point: the menu logic doesn't scale well. What do you have to change when you need to add a third option? You need to adjust the initial check and you need another else if branch:

  input.find_first_not_of("123qQ") // ...
  
  /* ... */ 
  
  else if (input == "3") { /* ... */ }
  

  The number of options to make this approach unreadable and hard to maintain is quite low, so think about an alternative here. To give you an idea of how this could be improved:

  struct MenuAction {
     std::string description;
     std::function<void()> action;
  };
  
  static const std::map<std::string, MenuAction> actionTable {
     {"1", { "Add entry", [](){ std::cout << "Add entry" << "\n";  }}},
     {"2", { "Edit entry", [](){ std::cout << "Edit entry" << "\n";  }}},
     {"q", { "Quit", [](){ std::cout << "Quit" << "\n";  }}}
  };
  

  Then, you can display available options (assuming C++17 is available) via

  for (const auto& [key, value] : actionTable)
     std::cout << key << ". " << value.description << "\n";
  

  and the original if-else if-else logic boils down to

  while (actionTable.count(input) == 0)
     input = GetInput();
  
  actionTable.at(input).action();
  

  Maintaining user options is now identical to maintaining the std::map instance.

• Minor detail; you naming scheme doesn't seem to be consistent. GetInput() starts with an upper case letter, while mainMenu() with a lower case letter.

• This is very rare, but you could use a do-while construct here to reduce the number of calls to GetInput():

   std::string input;
  
   do
      input = GetInput();
   while (actionTable.count(input) == 0);
  

  It seems at least debatable to me whether this is worth it, but as it crossed my mind, I couldn't resist suggesting it :)