Function to prompt and check user input

Question

I'm trying to make a function that asks for a string input and then validates it. If the answer is not correct, the function asks them again for valid input with a while loop.

In order to validate the string input, I'm using a string array with all the possible answers to the question.

Because I'm using a string array I need to pass it into the function as a pointer.

The problem I'm running into is that to call the function properly currently I have to type this:

std::string possibleAnswers[3] = { "0","1","2" };
//find the length of the possibleAnswers array
int length = sizeof(possibleAnswers) / sizeof(possibleAnswers[0]);
//create a pointer that points at the string array
std::string* pointerPossibleAnswers = possibleAnswers;
//call function to validate input
std::string response = checkInput(pointerPossibleAnswers, length);

You'll also notice that I used sizeof(possibleAnswers)/sizeof(possibleAnswers[0] to find the length which I can't call within the function, because the pointer in the function points to an element in the array, not the entire array. My function implementation is here:

std::string checkInput(std::string* pointerPossibleAnswers, int length)
{
 std::string input;
 //input = toLowerCase(input);
 //create a boolean to continue to ask for input
 bool isAsking = true;
 while (isAsking)
 {
 std::getline(std::cin, input);
 //for loop checks against possibleAnswer array
 for(int i = 0; i< length; i++)
 {
 // dereference possibleAnswers pointer to the
 //ith position of the array
 if (input == *pointerPossibleAnswers)
 {
 isAsking = false;
 //stop checking if a possible response is found
 //break from the loop on next iteration
 i = length;
 }
 //make the pointer point to the next spot
 pointerPossibleAnswers++;
 }
 if (isAsking == true)
 {
 std::cout << "Invalid response, please enter a valid response." << std::endl;
 //reset the pointer to point at original memory 
 //location
 for (int i = 0; i < length; i++)
 {
 pointerPossibleAnswers--;
 }
 }
 }
 return input;
}

I was just wondering if there is an easier way to call the function, which is one or two lines something like:

std::string possibleAnswers[3] = { "0","1","2" };
std::string response = checkInput(pointerPossibleAnswers, length);

within two lines that's more efficient, because I have noticed that I'm copying and pasting a lot of code each time. I feel like there is a much more efficient way to do this with a class or a vector, given they have some built in methods for calling

• πάντα ῥεῖ: What about using std::vector or std::array instead of a raw c-style array of std::string?
• George Smith: @πάντα ῥεῖ That's a good suggestion I'll try changing it to a vector so I can stuff more of it into a function
• Mast: Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers .

Answer 1

• Use std::vector<std::string> instead C-style array for possibleAnswers and then pass a reference to the vector into the checkInput function.

• If you will use std::vector you can use just std::find to check if the input appears in the possibleAnswers.

• Consider making possibleAnswers constant¹. It is never change, is not it?

• The sizeof operator returns a value of type std::size_t but you use int length to hold it. You should use std::size_t instead of int for the loop counters i inside the checkInput fucntion as well. Note that int is not guaranted to be large enough to hold a size of any object type (an array of std::string in your case).

• It is a good habit to prefer pre-increment over post-increment all things being equal in C++ since the first usually is faster.

• Do not comment your code too much. Anyone perfectly knows that * means dereference:

  // dereference possibleAnswers pointer to the
  //ith position of the array
  if (input == *pointerPossibleAnswers)
  

  Make it a rule: "Comments shouldn't say what happens in code, they should say why this is happens".

• You can easily restructure your code to get rid of the isAsking flag variable. All you have to do is explicitly return input when it equals to one of the possible answer.

---

_{¹ As mentioned @πάντα ῥεῖ in the comments if you'll decide to use to make possibleAnswers constant you may want to use std::array. But in this case you have to use templates to pass different sized arrays to the possibleAnswers function.}

• πάντα ῥεῖ: "It is a good habit to prefer pre-increment over post-increment ..." That's a long standing myth still told, while no longer really valid for modern optimizing compilers. I also still prefer pre-incerement though.
• eanmos: @πάνταῥεῖ, but it is still a good habit, especially if you are forced to use a dinosaur compiler.
• πάντα ῥεῖ: Sure, I just wanted to mention that performance isn't an argument anymore for most modern compilers.
• George Smith: @eanmos Thanks for the suggestions, I apologise for my inexperience. I think I'll try using templates, given const vectors are a little redundant. I genuinely was not aware of std::size_t nor pre-increment. I'll cut down on the comments as well
• George Smith: @eanmos one point of clarification, if I get rid of the boolean isAsking I'm just thing that the other option would be to have a while(true) loop which I've always been advised is bad practice, is there another option which would be a little better?

Answer 2

Include the necessary standard headers

To compile successfully, this code needs at least

#include <iostream>
#include <string>

Don't use signed types for size

Prefer std::size_t length.

Don't compare booleans to true or false

A boolean that's not false must be true, so just write

 if (isAsking)

Consider using an infinite loop

Return from within the loop when we have a matching answer, instead of having to maintain the isAsking variable. That also enables us to reduce the scope of input.

• George Smith: Thanks for the suggestions I messed up with the booleans and signed types, just with the infinite loop, I was just wondering if there was a better way to do that other than while(true), because I've always been advised not to do that?
• Toby Speight: I'd say that while (true) is exactly the right way to implement such a loop; I don't think the advice was particularly good. You do want to take care that the control flow is understandable, but I don't expect that to be a problem here.