patterncppMinor
Semi-complex Guess My Number
Viewed 0 times
numbersemicomplexguess
Problem
Please understand my request before commenting, I'm looking for positive critiques of my work. Anything I've done which could be done better while using the same, or similar, restrictions on data types and #include files that I've used. To put it another way, if you see anything that could be done more efficiently, using the same restrictions, please advise.
I know there's more elegant ways to do this. I'm just trying to push my understanding of the basic C++ constructs, before getting into more advanced C++ implementations.
code:
```
//02_ex_03_guess_my_number_aplha
#include
#include
#include
using namespace std;
int main()
{
bool exitGame = false;
bool playAgain = true;
bool roundOver = false;
char playAgainYes = 'y';
int playModeSelect = 0;
int computersNumber = 0;
int tooLow = 0;
int tooHigh = 101;
int playersNumber = 0;
int tries = 0;
enum playMode { playerVersusCpu = 1, cpuVersusPlayer, exitMenu};
srand(static_cast(time(0)));
cout > playModeSelect;
switch (playModeSelect)
{
case playerVersusCpu:
cout > playersNumber;
if (playersNumber > computersNumber)
{
cout > playAgainYes;
if (playAgainYes == 'y')
{
playAgain = true;
cout > playersNumber;
tries = 0;
tooLow = 0;
tooHigh = 101;
do
{
computersNumber = rand() % 100 + 1;
while ((computersNumber != playersNumber) && (computersNumber > tooLow) && (computersNumber tooLow) && (computersNumber playersNumber))
{
tooHigh = computersNumber;
}
}
} while (computersNumber
I know there's more elegant ways to do this. I'm just trying to push my understanding of the basic C++ constructs, before getting into more advanced C++ implementations.
code:
```
//02_ex_03_guess_my_number_aplha
#include
#include
#include
using namespace std;
int main()
{
bool exitGame = false;
bool playAgain = true;
bool roundOver = false;
char playAgainYes = 'y';
int playModeSelect = 0;
int computersNumber = 0;
int tooLow = 0;
int tooHigh = 101;
int playersNumber = 0;
int tries = 0;
enum playMode { playerVersusCpu = 1, cpuVersusPlayer, exitMenu};
srand(static_cast(time(0)));
cout > playModeSelect;
switch (playModeSelect)
{
case playerVersusCpu:
cout > playersNumber;
if (playersNumber > computersNumber)
{
cout > playAgainYes;
if (playAgainYes == 'y')
{
playAgain = true;
cout > playersNumber;
tries = 0;
tooLow = 0;
tooHigh = 101;
do
{
computersNumber = rand() % 100 + 1;
while ((computersNumber != playersNumber) && (computersNumber > tooLow) && (computersNumber tooLow) && (computersNumber playersNumber))
{
tooHigh = computersNumber;
}
}
} while (computersNumber
Solution
Starting with a few simple things:
It's a poor idea to import the whole of
When using
Two variables are declared but never used:
On to the structure. The code looks like One Big Main(). It really helps to identify self-contained chunks of functionality and, especially, any re-usable parts of the program. So I'd write something more like:
Having done that, we can implement the functions
We can be more rigorous about ensuring that the displayed numbers match how we interpret them:
Note that I've explicitly flushed the output stream before reading any input. This should happen anyway when switching between
Now let's move on to
Some things that I see here: in the if/else-if cascade, the last condition will always be true if the first two were false, so we can write a simple
You'll see that I made
Finally, let's have the computer do the guessing:
```
void playPlayersChoice()
{
int playersNumber;
std::cout > playersNumber;
int tries = 0;
int tooLow = 0;
int tooHigh = 101;
int computersNumber;
do {
computersNumber = rand() % 100 + 1;
while ((computersNumber != playersNumber) && (computersNumber > tooLow) && (computersNumber tooLow) && (computersNumber playersNumber)) {
tooHigh = computersNumber;
}
}
} while (computersNumber != playersNumber);
std::cout << "We
using namespace std;It's a poor idea to import the whole of
std namespace into your program. Instead, I suggest you selectively take just those names you intend to use.system("pause");When using
system(), you should always check its return value; in this particular case, it will always fail unless there's a pause program on the user's PATH. I have no such thing, and it doesn't seem to be in any common Linux distribution I've ever seen - it appears to be this anti-pattern.Two variables are declared but never used:
playAgain and roundOver.On to the structure. The code looks like One Big Main(). It really helps to identify self-contained chunks of functionality and, especially, any re-usable parts of the program. So I'd write something more like:
int main()
{
playMode playModeSelect;
do {
playModeSelect = selectPlayMode();
switch (playModeSelect) {
case playerVersusCpu:
playComputersChoice();
break;
case cpuVersusPlayer:
playPlayersChoice();
break;
case exitMenu:
// do nothing here
break;
}
} while (playModeSelect != exitMenu);
}Having done that, we can implement the functions
selectPlayMode(), playComputersChoice() and playPlayersChoice(). Let's start with selectPlayMode(). A function version of your code looks like:playMode selectPlayMode()
{
using std::cout;
using std::cin;
cout > playModeSelect;
return playMode(playModeSelect);
}We can be more rigorous about ensuring that the displayed numbers match how we interpret them:
cout << playerVersusCpu << " - Player Versus Computer\n";
cout << cpuVersusPlayer << " - Computer Versus Player\n";
cout << exitMenu << " - Exit\n";
cout << "Select a game mode: " << std::flush;Note that I've explicitly flushed the output stream before reading any input. This should happen anyway when switching between
cout and cin, but it's good to be clear that this is what we want.Now let's move on to
playComputersChoice(). If we extract this into a function, we have:void playComputersChoice()
{
int computersNumber = (rand()) % 100 + 1;
int tries = 0;
std::cout > playersNumber;
if (playersNumber > computersNumber)
{
std::cout << "Too bad. Your guess was too high.\n";
std::cout << "Try again: " << std::flush;
++tries;
}
else if (playersNumber < computersNumber)
{
std::cout << "Too bad. Your guess was too low.\n";
std::cout << "Try again: " << std::flush;
++tries;
}
else if (playersNumber = computersNumber)
{
std::cout << "You did it. You guessed my number.\n";
++tries;
std::cout << "It only took you " << tries << " tries.\n";
}
} while (playersNumber != computersNumber);
}Some things that I see here: in the if/else-if cascade, the last condition will always be true if the first two were false, so we can write a simple
else there. Note also that we increment tries in each branch of the if, so we can hoist that statement so it's always executed. Finally, the do/while repeats the test of playersNumber against computersNumber; instead of that, we could write an infinite loop, and break out of it when the player wins:void playComputersChoice()
{
const int computersNumber = (rand()) % 100 + 1;
std::cout > playersNumber;
if (playersNumber > computersNumber) {
std::cout << "Too bad. Your guess was too high.\n"
<< "Try again: " << std::flush;
} else if (playersNumber < computersNumber) {
std::cout << "Too bad. Your guess was too low.\n"
<< "Try again: " << std::flush;
} else {
std::cout << "You did it. You guessed my number.\n"
<< "It only took you " << tries << " tries.\n";
return;
}
}
}You'll see that I made
computersNumber constant, to show that during this game, the computer can't change its secret number - that would be cheating! More seriously, the judicious use of const can help to avoid misunderstandings and mistakes.Finally, let's have the computer do the guessing:
```
void playPlayersChoice()
{
int playersNumber;
std::cout > playersNumber;
int tries = 0;
int tooLow = 0;
int tooHigh = 101;
int computersNumber;
do {
computersNumber = rand() % 100 + 1;
while ((computersNumber != playersNumber) && (computersNumber > tooLow) && (computersNumber tooLow) && (computersNumber playersNumber)) {
tooHigh = computersNumber;
}
}
} while (computersNumber != playersNumber);
std::cout << "We
Code Snippets
using namespace std;system("pause");int main()
{
playMode playModeSelect;
do {
playModeSelect = selectPlayMode();
switch (playModeSelect) {
case playerVersusCpu:
playComputersChoice();
break;
case cpuVersusPlayer:
playPlayersChoice();
break;
case exitMenu:
// do nothing here
break;
}
} while (playModeSelect != exitMenu);
}playMode selectPlayMode()
{
using std::cout;
using std::cin;
cout << "1 - Player Versus Computer\n";
cout << "2 - Computer Versus Player\n";
cout << "3 - Exit\n";
cout << "Select a game mode: ";
int playModeSelect;
cin >> playModeSelect;
return playMode(playModeSelect);
}cout << playerVersusCpu << " - Player Versus Computer\n";
cout << cpuVersusPlayer << " - Computer Versus Player\n";
cout << exitMenu << " - Exit\n";
cout << "Select a game mode: " << std::flush;Context
StackExchange Code Review Q#156001, answer score: 5
Revisions (0)
No revisions yet.