[C++] Problem with Break inside of an If statement inside of a Do While loop

[Da)sniper]

Code

void PhraseGen::output(string inputPhrase[], string inputDef[], int terms)
{
	int index, counter = 0;
	string phrase;
	string restart, yesOrNo;
	do
	{
	do
	{
	srand (time(NULL));
	index = rand() % terms;
	cout << "What is:  " << inputPhrase[index] << "? ";
	cin >> phrase;
	cout << endl;
	if(inputDef[index] == phrase)
		cout << "You're correct!" << endl << endl;
	else
	{
		cout << "That's wrong try again" << endl << endl;
		counter++;
		if(counter >= 5)
			cout << endl << endl << "You seem to be having a hard time with this question.  Try studying more and" << endl <<  "then coming back to test yourself" << endl << endl;
		
	}
	} while(inputDef[index] != phrase);
	cout << "Would you like to pick a new phrase? (Yes/No):  ";
		cin >> yesOrNo;
		cout << yesOrNo;
	if(yesOrNo == "No" || "N" || "n" || "no")
			(true);
	if(yesOrNo == "Yes" || "yes" || "y" || "Y")
			break;

	}while (true);
}

So, in essence my program takes a set of words and definitions the user inputs to the system, randomizes the word and asks for the definition. (To help someone study) This is one of the functions for my class; PhraseGen. Yes I know this is kind of poor coding.

But for some reason there's a logical error at the bottom two if statements.
If you type in "Yes" then it will simply end the program without warning and return 0. It will do the same if you type "No".

After working with it for about a few hours, I was able to make it switch back and forth between "Yes" work for both Yes and No, and "No" do the same.

 

[GhostWolf]

The first thing to do would be to make proper indentation so your code can actually be read.

I am such a nice guy that I did it for you, and added comments for you to read.

void PhraseGen::output(string inputPhrase[], string inputDef[], int terms)
{
  int index, counter = 0;
  string phrase;
  string restart, yesOrNo;
  
  do
  {
    // This whole block should be in a function, since you want to run it again if the user answers "Yes" later on
    do
    {
      srand(time(NULL)); // You don't need to keep feeding the RNG with a new seed, do it once at the start of the program
      index = rand() % terms;
      cout << "What is:  " << inputPhrase[index] << "? ";
      cin >> phrase;
      cout << endl; // Don't use endl all over the place
      
      if (inputDef[index] == phrase)
      {
        cout << "You're correct!" << endl << endl; // Don't use endl all over the place
      }
      else
      {
        cout << "That's wrong try again" << endl << endl; // Don't use endl all over the place
        counter++; // You are incrementing counter, but never set it back to 0 (which didn't effect you because you are not actually using this block twice). Set it to 0  before this loop.
        
        if (counter >= 5)
        {
          cout << endl << endl << "You seem to be having a hard time with this question.  Try studying more and" << endl <<  "then coming back to test yourself" << endl << endl; // Don't use endl all over the place
        }
      }
    } while (inputDef[index] != phrase);
    
    cout << "Would you like to pick a new phrase? (Yes/No):  ";
    cin >> yesOrNo;
    cout << yesOrNo;
    
    if (yesOrNo == "No" || "N" || "n" || "no") // "N", "n" and "no" are string objects and are evaluated to "true", so this condition is alwasy true
    {
        (true); // What exactly is this supposed to do?? This is just an expression that evaluates to "true"
    }
    // This can be else if
    if (yesOrNo == "Yes" || "yes" || "y" || "Y") // "yes", "y" and "Y" are string objects and are evaluated to "true", so this condition is alwasy true
    {
        // This breaks from the main loop, isn't this supposed to be in the "No" option?
        break;
    }
  } while (true);
}

If you keep feeding the RNG with a new seed, it's not much of an RNG. Seed it once when the program starts, and then let it do its job by calling rand().

In general, try to surround blocks with the block characters {}, even when they have only one expression. You will avoid many arbitrary bugs and debugging in the future if you do.

Don't use std::endl without actually knowing what it does (apart from making a new line).
Unless you know that you specifically need std::endl, simply use the escape character \n.

When writing multiple expressions in a condition, remember that each one is evaluated on its own.
Having the condition if (a == 1 || 2) may look like it checks if a is equal to 1 or 2, but it in fact checks if the expression a==1 is true, and if a==1 is false then it goes on to check if the expression 2 is true, which always evaluates to true (since 2 evaluates to true in boolean expressions).

As general feedback:

Your code doesn't handle the case when a phrase was already selected and beaten (matched correctly), you might want to add that, by keeping track of all the matched phrases and rand() until you get something that wasn't matched (you could use std::map for this).

I don't know what sort of phrases you have, but you should do case-insensitive checks on the strings (here's an example on how to easily do that).

 

[Da)sniper]

Thank you so much, I just looked at the code I posted myself and realized that a lot of that was just me getting angry and randomly putting in values to see what would come of it.

I fixed it to a working point but still a few minor things that are still wrong / I'm curious about.

For starters, I was always told to use endl. No real rhyme or reason, just use it. Is there a reason to use \n over endl?

As well with the block characters. I used to always do that but I was told that it's just cumbersome with only one line for the statement. Any reasoning behind it?

I switched every array over to Vectors. Now, I know very little about vectors but currently, it's only set to the amount of terms they have claimed to have but it seems to have one empty data slot no matter the size of terms. Is there a way to get rid of this?

Also thank you again. You saved me a lot of trouble in trying to research how to pull that off.

 

[Kanadaj]

Imho only use endl as the last one, use "\n"s before, since endl flushes the buffer, meaning a lot more operation than necessary. Of course, for starters and 100 line codes, it pretty much doesn't matter.

When enumerating the size, vectors should show you the current number of elements. Capacity (and allocated memory) should always be bigger than the size.

EDIT: I think the traditional increment is 2^N, although, in case of compiling with Visual Studio 2013, the capacity has a strange increment: 1-2-3-4-6-9-13-19-28-42-63-94-141-211-316... I wonder why, and what kind of algorithm it uses to calculate the new capacity...

 

[Da)sniper]

Thanks. Also, I think I fixed it. I believe I messed up a core concept of for loops before.

I believe the algorithm is roughly n * 1.5 (But even then the rounding is really weird.) Probably so there were intermediate terms rather then going from 64 to 128.

 

[GhostWolf]

The reason to add braces even when you have only one statement after a condition is because you will later on not notice it's one statement, and the indentation would mislead you further.

It might seem silly now, but it will save you a lot of time when your code will become bigger and complex.

 

[Kanadaj]

well I only skip braces for short one-line statements, like

if (GetBool()) MyClass.DoSomething();