Thread: Looking for feedback on program segment

Heey, this is going to be my first post here, so be gentle
I have been working toward writing a text-based roleplaying game, and decided to stick this thread here, since this segment of the game is more related to C++ than game programming in general.
I'm hoping to get some feedback on how I did the user error checking throughout the program. As it is, it compiles great, but the huge amount of code needed for error checking leads me to believe there may have been a more efficient way of doing it. Any help would be appreciated. In advance, I apologize for the giant block of code. The implementation file I'm putting here wouldn't really make sense without its entirety.

Oh, and I'm pretty new to C++, so I'm sure my formatting could use some work. I'm working on it based on what I've seen other people do.

Code:

void CharCreate::CreateChar()
{
	gHasData = true;
	Body body;
	string tempStr;
	string choice;
	string tempString;
	string tempString1;
	string tempStr1;
	int temp;
	bool choices = false;
	bool isChoice = false;
	bool validChoice = false;
	bool validName = false;
	bool isValid = true;
	bool hasSpace = false;
	Output out;
	while(!choices)
	{
	choices = false;
	isChoice = false;
	while(!validName)
	{                              //out.Print is my word-wrap function. I couldn't find an
                                        // existing function that worked for this.
	out.Print("Welcome to character creation. Please enter a name for your character.\n");
	out.Arrow();
	getline(cin, tempString);
	out.End();
	if(tempString.length() > 25)
	{
		out.Print("You have entered an invalid name. Your name must be 25 characters or less.\n");
		isChoice = true;
		isValid = false;
		break;
	}
	if(tempString.length() < 4)
	{
		out.Print("You have entered an invalid name. Your name must be 4 characters or more.\n");
		isChoice = true;
		isValid = false;
		break;
	}
	for(int i = 0; i < tempString.length(); i++)
	{
		if(!isalpha(tempString[i]))
		{
			out.Print("Your name may only contain alphabetical characters.\n");
			isChoice = true;
			isValid = false;
			break;
		}
	}
	if(!isValid)
		break;
	tempString1 = tempString;
	tempString[0] = toupper(tempString1[0]);

	for(int i = 0; i < tempString.length() - 1; i++)
		tempString[i+1] = tolower(tempString1[i+1]);
	validName = true;
	}
	while(!isChoice)
	{
	out.Print("You have selected:\n");
	out.Print(tempString);
	out.End();
	out.Print("Is this correct?\n");
	out.Arrow();
	cin >> choice;
	cin.ignore();
	out.End();
	if(toupper(choice[0]) == 'Y')
	{
		choices = true;
		isChoice = true;
		validName = true;
		gPlayer.playName = tempString;
	}
	else if(toupper(choice[0]) == 'N')
	{	
		choices = false;
		isChoice = true;
		validName = false;
	}
	else
		out.Print("You have entered an invalid choice. Please enter 'Yes' or 'No'\n");	
	}
	}
	choices = false;
	isChoice = false;
	while(!isChoice)
	{
		while(!choices)
		{
		while(!validChoice)
		{
		out.Print("At any point during character creation, you may type 'Help'('H') or 'Restart'('R') to bring up more information or restart from this point.\n");
		out.Print("Race Selection:\n");
		out.Print("1)Human\n2)Elf\n3)Ogre\n4)Troll\n5)Halfling\n6)Drow\n7)Gnome\n8)Goblin\n9)Half-Orc\n10)Half-Giant\n11)Orc\n12)Giant\n13)Torako\n");
		out.Arrow();
		getline(cin, tempStr);
		out.End();
		switch(toupper(tempStr[0]))
		{
		case '1':
			{
				switch(toupper(tempStr[1]))
				{
				case '0': 
					validChoice = true;
					gPlayer.playRace = "Half-Giant";
					break;
				case '1': 
					validChoice = true;
					gPlayer.playRace = "Orc";
					break;
				case '2': 
					validChoice = true;
					gPlayer.playRace = "Giant";
					break;
				case '3': 
					validChoice = true;
					gPlayer.playRace = "Torako";
					break;
				default: 
					validChoice = true;
					gPlayer.playRace = "Human";
				}
				break;
			}
		case '2': 
			validChoice = true;
			gPlayer.playRace = "Elf";
			break;
		case '3': 
			validChoice = true;
			gPlayer.playRace = "Ogre";
			break;
		case '4': 
			validChoice = true;
			gPlayer.playRace = "Troll";
			break;
		case '5': 
			validChoice = true;
			gPlayer.playRace = "Halfling";
			break;
		case '6': 
			validChoice = true;
			gPlayer.playRace = "Drow";
			break;
		case '7': 
			validChoice = true;
			gPlayer.playRace = "Gnome";
			break;
		case '8': 
			validChoice = true;
			gPlayer.playRace = "Goblin";
			break;
		case '9': 
			validChoice = true;
			gPlayer.playRace = "Half-Orc";
			break;
		}
		int i = 0;
		if(toupper(tempStr[0]) == 'H')
		{
			tempStr1 = tempStr;
			for(i = 0; i < tempStr.length() - 1; i++)
			{
				if(tempStr[i] == ' ')
				{
					hasSpace = true;
					i--;
					break;
				}
				tempStr1[i] = tempStr[i];
			}
			tempStr1.erase(i+1, tempStr1.length() - (i + 1));
			i++;
			if(!hasSpace && toupper(tempStr[1]) == 'E')
				body.HelpCall("CharCreate");
			else if(tempStr.length() == 1)
				body.HelpCall("CharCreate");
			else if(!hasSpace && toupper(tempStr[1]) != 'E' && tempStr.length() != 1)
				out.Print("Invalid command.\n");
			else if(tempStr[i] == ' ' && !isalnum(tempStr[i+1]))
				out.Print("Invalid Command.\n");
			else if(tempStr[i] == ' ' && tempStr.length() == i)
				body.HelpCall("CharCreate");
			else if(tempStr[i] == ' ' && (tempStr[i+1] == '1' || tempStr[i+1] == '2' || tempStr[i+1] == '3' || tempStr[i+1] == '4' || 
										  tempStr[i+1] == '5' || tempStr[i+1] == '6' || tempStr[i+1] == '7' || tempStr[i+1] == '8' || 
										  tempStr[i+1] == '9'))
			{
				switch(tempStr[i+1])
				{
				case '1': switch(tempStr[i+2])
						  {
							case '0': body.HelpCall("Half-Giant");
								system("pause");
								break;
							case '1': body.HelpCall("Orc");
								system("pause");
								break;
							case '2': body.HelpCall("Giant");
								system("pause");
								break;
							case '3': body.HelpCall("Torako");
								system("pause");
								break;
							default: body.HelpCall("Human");
								system("pause");
								break;
							}
				case '2': body.HelpCall("Elf");
					system("pause");
					break;
				case '3': body.HelpCall("Ogre");
					system("pause");
					break;
				case '4': body.HelpCall("Troll");
					system("pause");
					break;
				case '5': body.HelpCall("Halfling");
					system("pause");
					break;
				case '6': body.HelpCall("Drow");
					system("pause");
					break;
				case '7': body.HelpCall("Gnome");
					system("pause");
					break;
				case '8': body.HelpCall("Goblin");
					system("pause");
					break;
				case '9': body.HelpCall("Half-Orc");
					system("pause");
				}
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'H' &&
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'U' &&
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'M' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'A' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == 'N'))
			{
				body.HelpCall("Human");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'H' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'A' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'L' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'F' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == '-' && 
										  (char)toupper(tempStr[tempStr1.length()+6]) == 'G' && 
										  (char)toupper(tempStr[tempStr1.length()+7]) == 'I' && 
										  (char)toupper(tempStr[tempStr1.length()+8]) == 'A' && 
										  (char)toupper(tempStr[tempStr1.length()+9]) == 'N' && 
										  (char)toupper(tempStr[tempStr1.length()+10]) == 'T'))
			{
				body.HelpCall("Half-Giant");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'O' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'R' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'C'))
			{
				body.HelpCall("Orc");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'G' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'I' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'A' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'N' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == 'T'))
			{
				body.HelpCall("Giant");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'T' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'O' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'R' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'A' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == 'K' && 
										  (char)toupper(tempStr[tempStr1.length()+6]) == 'O'))
			{
				body.HelpCall("Torako");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'E' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'L' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'F'))
			{
				body.HelpCall("Elf");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'O' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'G' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'R' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'E'))
			{
				body.HelpCall("Ogre");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'T' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'R' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'O' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'L' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == 'L'))
			{
				body.HelpCall("Troll");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'H' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'A' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'L' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'F' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == 'L' && 
										  (char)toupper(tempStr[tempStr1.length()+6]) == 'I' && 
										  (char)toupper(tempStr[tempStr1.length()+7]) == 'N' && 
										  (char)toupper(tempStr[tempStr1.length()+8]) == 'G'))
			{
				body.HelpCall("Halfling");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'D' &&
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'R' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'O' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'W'))
			{
				body.HelpCall("Drow");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'G' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'N' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'O' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'M' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == 'E'))
			{
				body.HelpCall("Gnome");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'G' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'O' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'B' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'L' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == 'I' && 
										  (char)toupper(tempStr[tempStr1.length()+6]) == 'N'))
			{
				body.HelpCall("Goblin");
				system("pause");
			}
			else if(tempStr[i] == ' ' && ((char)toupper(tempStr[tempStr1.length()+1]) == 'H' && 
										  (char)toupper(tempStr[tempStr1.length()+2]) == 'A' && 
										  (char)toupper(tempStr[tempStr1.length()+3]) == 'L' && 
										  (char)toupper(tempStr[tempStr1.length()+4]) == 'F' && 
										  (char)toupper(tempStr[tempStr1.length()+5]) == '-' && 
										  (char)toupper(tempStr[tempStr1.length()+6]) == 'O' && 
										  (char)toupper(tempStr[tempStr1.length()+7]) == 'R' && 
										  (char)toupper(tempStr[tempStr1.length()+8]) == 'C'))
			{
				body.HelpCall("Half-Orc");
				system("pause");
			}
			else
				out.Print("Invalid Command.\n");
		}
		if(toupper(tempStr[0]) == 'R')
		break;
		out.Print("You have selected:\n");
		out.Print(gPlayer.playRace);
		out.Print("\n");
		}
		break;
		}
		break;
		}
}

The only thing I notice immediately is that this function is doing way too much work. Split it up into a few smaller functions. If your function is longer than 100 lines or passes the 3rd or 4th indentation level, these are good signs that it's too complex. This will make everything quite a bit easier to manage, it should even clarify the efficiency problem.

First, your indentation is not consistent. That may be because of the horizontal size of the code, but you should still make sure to add an indentation level for each nested block of code.

It is generally considered better practice to declare variables only when you are ready and need to use them, instead of putting them all at the beginning of the function. This is mostly a style issue but it can make your code better.

You should consider breaking that code down into more functions. A good candidate for a new function is all the code inside a large loop or if block. Just take the whole chunk out, put it in a new function, and identify what variables need to be passed and/or returned. In some cases the parameter passing can get complicated, but it is worth it to identify and fix those situations.

>> (char)toupper(tempStr[tempStr1.length()+1]) == 'H'
The comparisons like this don't make a lot of sense. Why not just convert all characters to uppercase and then compare with operator==? This would greatly reduce the amount of code. In fact, you should be able to just check to see if the command is valid (a set of valid commands would be useful there) and then send it directly to HelpCall in one set of code, instead of a separate if block for each command.

Thanks for your replies! What you're saying about breaking it into more functions makes sense. And I had always believed it was better practice to put all variables at the beginning of the function. I'll work on all that a bit and update the post. And thanks for the feedback on my formatting, I've been trying to figure out the standards on that.

>> And I had always believed it was better practice to put all variables at the beginning of the function.
That was required in C, and had been brought over as good practice for C++, but with RAII and other C++ techniques it actually makes less sense.