patterncppMinor
Assigning stats in a game
Viewed 0 times
gamestatsassigning
Problem
I made a program that allows you to assign stats. However it uses some fairly complex if-else statements. Also, someone told me that
goto statements were bad, but why is that? I know that switch might have been a better solution, but I cannot get switch statements to work for string variables.#include
int main()
{
using namespace std;
int strength,agility,perception,endurance,intelligence,rem_statpoints;
cout > inc;
if (inc == "strength" or "agility" or "perception" or "endurance" or "intelligence") {
goto increase;
} else {
cout > ass_statpoints;
if (ass_statpoints > rem_statpoints) {
cout > red;
string statreset; //string statreset is placed here because I could not place it next to the relevant cin for some reason.
if (red == "play!") {
goto game;
} else {
cout > statreset; //string statreset is placed here
if (statreset == "yes") {
cout > statresetno;
if (statresetno == "yes") {
goto game;
} else {
cout << "Make up you mind!" << endl;
goto red;
}
}
}
goto stat;
game:{}
{
cout << "Initializing game" << endl;
cout << "game coming soon!" << endl;
cout << "Thank you for playing!" << endl;
}
return 0;
}
/* Some one told me once that is was bad to use alot of goto statements,
* however, I can not see what damage they are causing here.
* Then again, I am not familiar with any alternatives for goto and labels,
* so I am forced to use goto!
* I really hoped you enjoyed using my program, and if this program for
* some reason helped someone else, I am glad.
* Feel free to use this code for yourself if you really found it that
* good
*
*
* Happy Trails!
* - Lemonizer
*/Solution
Please put one variable per line:
That is really hard to read:
It would be even better to wrap this in a structure (one day you may have 2 players at the same time).
Would it not be easier to make this a function/method:
This looks like another mehtod to me:
This is not doing what you think it is doing.
I am surprised if this even works as it should always return true.
The comment at the end:
I can.
You are tightly binding your control flow logic.
This makes it hard to introduce new steps or alter the logic.
Following the logic in this code is even worse. It is the perfect example of spaghetti. If it works fine then great. But try following the logic when something breaks. This becomes a maintenance nightmare.
Alternatives:
Combine the following:
You can reduce the complexity of you of tree like this:
I would restructure the code like this:
Then generatePlayer()
int strength,agility,perception,endurance,intelligence,rem_statpoints;That is really hard to read:
It would be even better to wrap this in a structure (one day you may have 2 players at the same time).
struct Player
{
int strength;
int agility;
int perception;
int endurance;
int intelligence;
};
int rem_statpoints; // not part of a player.
// as it is just used during player creationWould it not be easier to make this a function/method:
statreset:{}
strength = 5; //Strength is set to 5 by default
agility = 5; //Agility is set to 5 by default
perception = 5; //Perception is set to 5 by default
endurance = 5; //Endurance is set to 5 by default
intelligence = 5; //Intelligence is set to 5 by default
rem_statpoints = 10; //The avaivable number of stat points
void Player::statreset(int& rem_statpoints)
{
strength = 5; //Strength is set to 5 by default
agility = 5; //Agility is set to 5 by default
perception = 5; //Perception is set to 5 by default
endurance = 5; //Endurance is set to 5 by default
intelligence = 5; //Intelligence is set to 5 by default
rem_statpoints = 10; //The avaivable number of stat points
}This looks like another mehtod to me:
stat:{}
cout << "Strength " << strength << endl;
cout << "Agility " << agility << endl;
cout << "Perception " << perception << endl;
cout << "Endurance " << endurance << endl;
cout << "Intelligence " << intelligence << "\n" << endl;
cout << "What stat do you want to increase?" << endl;This is not doing what you think it is doing.
I am surprised if this even works as it should always return true.
string inc;
cin >> inc;
if (inc == "strength" or "agility" or "perception" or "endurance" or "intelligence")
// what you really want is:
if (inc == "strength" || inc == "agility" || inc == "perception" ... etcThe comment at the end:
/* Some one told me once that is was bad to use alot of goto statements,
* however, I can not see what damage they are causing here.I can.
You are tightly binding your control flow logic.
This makes it hard to introduce new steps or alter the logic.
Following the logic in this code is even worse. It is the perfect example of spaghetti. If it works fine then great. But try following the logic when something breaks. This becomes a maintenance nightmare.
* Then again, I am not familiar with any alternatives for goto and labels,
* so I am forced to use goto!Alternatives:
Combine the following:
if () {} else {}
for(;;) {}
while() {}
do {} while()
()You can reduce the complexity of you of tree like this:
if ()
{
}
else if ()
{
}
else if ()
{
}
else if ()
{
}
// ...etc
else
{
}I would restructure the code like this:
int main()
{
Player player1;
do
{
generatePlayer(player1);
}
while(!isUserSatisfied());
playGame();
}Then generatePlayer()
void generatePlayer(Player& player)
{
int extraPoints;
player.statreset(extraPoints);
while(extraPoints > 0)
{
std::cout > inc;
}
while(!isValidAttribute(inc));
do
{
std::cout > amount;
}
while(amount > extraPoints);
player.modifyAttribute(inc, amount);
}
}Code Snippets
int strength,agility,perception,endurance,intelligence,rem_statpoints;struct Player
{
int strength;
int agility;
int perception;
int endurance;
int intelligence;
};
int rem_statpoints; // not part of a player.
// as it is just used during player creationstatreset:{}
strength = 5; //Strength is set to 5 by default
agility = 5; //Agility is set to 5 by default
perception = 5; //Perception is set to 5 by default
endurance = 5; //Endurance is set to 5 by default
intelligence = 5; //Intelligence is set to 5 by default
rem_statpoints = 10; //The avaivable number of stat points
void Player::statreset(int& rem_statpoints)
{
strength = 5; //Strength is set to 5 by default
agility = 5; //Agility is set to 5 by default
perception = 5; //Perception is set to 5 by default
endurance = 5; //Endurance is set to 5 by default
intelligence = 5; //Intelligence is set to 5 by default
rem_statpoints = 10; //The avaivable number of stat points
}stat:{}
cout << "Strength " << strength << endl;
cout << "Agility " << agility << endl;
cout << "Perception " << perception << endl;
cout << "Endurance " << endurance << endl;
cout << "Intelligence " << intelligence << "\n" << endl;
cout << "What stat do you want to increase?" << endl;string inc;
cin >> inc;
if (inc == "strength" or "agility" or "perception" or "endurance" or "intelligence")
// what you really want is:
if (inc == "strength" || inc == "agility" || inc == "perception" ... etcContext
StackExchange Code Review Q#23372, answer score: 8
Revisions (0)
No revisions yet.