Prompt user to choose from menu

Question

The following code simply prompts the user to choose from a menu, and then it displays the choice made by that user.

It only accepts input consisting of a single digit between 1 and 6, inclusive. Anything else will be considered as being invalid.

Questions

1. While it considers Ctrl-D as an invalid choice, should it be handled differently? If so, how?

2. It takes user input as an int. Would you suggest taking it as a string instead, and then use either stoi or stringstream to convert it to an int?

3. Can it be re-written in a more elegant and concise way?

4. Do you see any potential issues?

5. Any comments?

Source Code

#include <iostream>
#include <limits>

int getChoice()
{
    std::cout << "Main Menu\n"
        << "-------------------\n"
        << "1. Add record\n"
        << "2. Edit record\n"
        << "3. Delete record\n"
        << "4. View record\n"
        << "5. View all records\n"
        << "6. Exit\n";
        

    bool isValid;
    int choice;

    do
    {
        isValid = true;
        std::cout << "Enter choice: ";
        std::cin >> choice;

        if (std::cin.fail())
        {
            if (std::cin.eof())
            {
                std::cin.clear();
            }
            else
            {
                std::cin.clear();
                std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            }
            isValid = false;
        }
        else
        {
            std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
            if (std::cin.gcount() > 1)
            {
                isValid = false;
            }
            else if (choice < 1 || choice > 6)
            {
                isValid = false;
            }
        }
        if (!isValid)
        {
            std::cout << "Invalid choice\n";
        }
    } while (!isValid);

    return choice;
}

int main()
{
    int choice;
    choice = getChoice();

    std::cout << "User chose option " << choice << std::endl;

    return 0;
}

Input and Results

Main Menu
-------------------
1. Add record
2. Edit record
3. Delete record
4. View record
5. View all records
6. Exit
Enter choice: x
Invalid choice
Enter choice: 8
Invalid choice
Enter choice: 2x
Invalid choice
Enter choice: x2
Invalid choice
Enter choice: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
Invalid choice
Enter choice: 888888888888888888888888888888888888888888888888888888888888888888888888888
Invalid choice
Enter choice: Invalid choice // Ctrl-D entered here
Enter choice: ^Z
[1]+  Stopped                 ./getchoice_cpp

Answer 1

Answers to your questions

While it considers Ctrl-D as an invalid choice, should it be handled differently? If so, how?

You should treat the end of the file as the definite end of the input, and return immediately with a value that indicates a failure. Not all EOFs are Ctrl-D's, there is no guarantee that you can even continue reading from std::cin after you encounter an EOF; for example the user might have redirected standard input to a file.

It takes user input as an int. Would you suggest taking it as a string instead, and then use either stoi or stringstream to convert it to an int?

As you have probably seen std::cin >> some_int does not read a whole line, instead it tries to read an integer but stops as soon as it encounters something that is not an integer. Instead of fighting against this, use std::getline() if you want to read a line, and then indeed use std::stoi() or something similar to convert that to a number. You still need to deal with the fact that also stoi() stops as soon as it sees the first character that is not part of an integer.

Can it be re-written in a more elegant and concise way?

Yes, see below.

Do you see any potential issues?

There is code duplication and unnecessary complexity dealing with std::cin. Also, the function is not very generic; if you want to get a choice for a different menu you would have to write a new function.

Reading a line with an integer in it

First, read a single line, and check if that succeeded. If not, either return some value indicating an error, or throw an exception. For example:

std::string line;

if (!std::getline(std::cin, line)) {
    throw std::runtime_error("Error while reading input");
}

Then try to convert it with std::stoi() or any of its variants, depending on the type of integer. Use the second argument to std::stoi() to know where it stopped parsing the integer, and check that this was at the end of the string:

try {
    size_t endpos;
    size_t value = stoi(line, &endpos);

    if (endpos == line.size()) {
        // Success!
        ...
    }
} catch (std::logic_error &) {
    // Error parsing the integer
    ...
}

Make getChoice() more generic

You hardcoded the choices in getChoice(). If you have a program with multiple menus, then you would have to write different versions of getChoice(), duplicating a lot of code. So try to make it more generic. For example, you can have getChoice() take two parameters: one is a string with the title of the menu, the second is a vector of strings with the possible choices.

Full example

Here is an example of what it could look like:

#include <iostream>
#include <stdexcept>
#include <string>
#include <vector>

size_t getChoice(const std::string &title, const std::vector<std::string> &choices)
{
    std::cout << title << "\n"
              << "-------------------\n";

    for (size_t i = 0; i < choices.size(); ++i) {
        std::cout << i << ". " << choices[i] << "\n";
    }

    while (true) {
        std::cout << "Enter choice: " << std::flush;
        std::string line;

        if (!std::getline(std::cin, line)) {
            throw std::runtime_error("Could not read input");
        }

        try {
            size_t endpos;
            size_t choice = stoul(line, &endpos);

            if (endpos == line.size() && choice < choices.size()) {
                return choice;
            }
        } catch (std::exception &) {
            // nothing to do here
        }

        std::cout << "Invalid choice\n";
    }
}

int main()
{
    static const std::vector<std::string> choices = {
        "Add record",
        "Edit record",
        "Delete record",
        "View record",
        "view all records",
        "Exit",
    };

    size_t choice = getChoice("Main menu", choices);

    std::cout << "User chose option " << choice << std::endl;
}

Note that I made the option numbers start with zero, just to keep the code slightly simpler. I also used size_t, as it is unsigned (no need to deal with negative numbers), and it's good practice whenever you deal with indices into arrays, vectors and so on. Note that you don't have to create a named vector in main(), you can even pass a literal to getChoices():

size_t choice = getChoice("Menu", {"First option", "Second option", ...});