std::getline valid input check doesn't work if false is ever returned

[ICantC]

hello, I have the below code which asks the user for input into a string, then checks whether that string is present in a vector of strings (member of class) to make sure the guess is valid. It all works great if you put valid guesses in, but the moment you put an invalid guess, which prompts the INVALID PASSWORD msg to display, even if you enter a correct guess after, you will always get an invalid password.

I don't understand why that is since I do cin.clear, cin.ignore and getline calls s.erase()?

bool computer::terminal::guess_valid(std::string guess)
{
	bool valid{ false };
	auto it = passwds.begin();
	while (it != passwds.end())
	{
		if (*it == guess)
		{
			valid = true;
			break;
		}
		++it;
	}

	if (valid == false)
		return false;
	else
		return true;
}

void computer::terminal::guess()
{
	std::cout << "\n\n> ";
	std::string s{};
	std::cin.ignore();
	std::getline(std::cin, s);

	while (!guess_valid(s))
	{
		print("\n\nINVALID PASSWORD");
		std::this_thread::sleep_for(std::chrono::seconds(2));
		std::cout << "\n\n> ";
		std::cin.clear();
		std::cin.ignore();
		std::getline(std::cin, s);
	}
}

[jlb]

Do you understand by that ignore() ignores a single character by default? Perhaps you should ignore the entire line instead of a single character.

Also when dealing with a string or a single character from cin the only "normal" cin failure condition is someone entering eof() instead of a string or character.

Have you considered running the program with your debugger, single stepping through the problem sections watching the variables as you step?

Lastly your guess_valid() function seems overly complicated.

[Ganado]

It would help us help you if you made a minimal, compileable example that reproduced your issue. As it is now, we have to take the effort to fill in the gaps ourselves.

Anyway, you shouldn't just put cin.ignore() calls everywhere. If you're only ever using getline and not std::cin >> operator, then you don't need cin.ignore() calls at all under normal circumstances.

If you used a --> debugger <--, you would see that the value of s after your line 28 would be "ello" if you typed "hello".
Remove the cin.ignore() calls.

[ICantC]

Thanks Ganado, removing the cin.ignore() in the while (!guess_valid(s)) loop fixed the issue.

jlb, when you see the guess_valid() is overly complicated, can you suggest a way to shorten it?

the class member it iterates through is std::vector<std::string> passwds;

which contains up to 50 strings, the user input must match one of those

[Ganado]

ICantC,

bool computer::terminal::guess_valid(std::string guess)
{
    for (const auto& str : passwds)
    {
        if (guess == str)
            return true;
    }
    return false;
}

Could probably be shortened even further with something from <algorithm>.

Edit:
http://www.cplusplus.com/reference/algorithm/any_of/

// Example program
#include <iostream>
#include <string>
#include <vector>
#include <algorithm>

const std::vector<std::string> passwds { "hello", "world", "password123" };

bool guess_valid(std::string guess)
{
    auto validity_test = [&guess](const std::string& pw) { return guess == pw; };
    return std::any_of(passwds.begin(), passwds.end(), validity_test);
}

int main()
{
    std::cout << guess_valid("world") << std::endl;
    std::cout << guess_valid("badpass") << std::endl;
}

Less lines of code, but I probably would still prefer the first one.

[ICantC]

Great!

thanks a lot, i'll have a good look at these and re-write my checks

[jlb]

And by the way passing a std::string by value is often considered a bad practice, instead consider either passing a std::string view or passing the string by const reference.

[mbozzi]

Just std::count would work:

bool guess_valid(std::string guess)
{
    return std::count(passwds.begin(), passwds.end(), guess);
}

Instead of

if (valid == false)
  return false;
else
  return true;

Just say
return valid;