2
\$\begingroup\$

I'm a beginner in programming and made a pretty simple heads or tails type game. I would like to know if I have done this efficiently. If not, please let me know what you would have done differently.

#include <iostream>
#include <cstdlib>
#include <ctime>
using namespace std;

void headsOrTails();
int randomNumber();

int main()
{
    headsOrTails();
}

int randomNumber()
{
    int number;
    srand(time(0));
    for(int i = 1; i < 2; i++)
    {
        number = 1 + (rand()%2);
    }
    return number;
}

void headsOrTails()
{
    string prediction;
    cout << "Heads or Tails? ";
    cin >> prediction;

    if((prediction == "Heads") || (prediction == "heads")){
        if(randomNumber() == 1){
            cout << "Heads, you are correct!\n" << endl;
        }
        else if(randomNumber() == 2){
            cout << "Tails, you were wrong.\n" << endl;
        }
    }
    if((prediction == "Tails") || (prediction == "tails")){
        if(randomNumber() == 1){
            cout << "Heads, you were wrong.\n" << endl;
        }
        else if(randomNumber() == 2){
            cout << "Tails, you are correct!\n" << endl;
        }
    }

    char playAgain;
    cout << "Play again?(Y/N) ";
    cin >> playAgain;
    cout << "\n";

    while((playAgain == 'Y') || (playAgain == 'y')){
        headsOrTails();
    }
    while((playAgain == 'N') || (playAgain == 'n')){
        exit(EXIT_FAILURE);
    }
}
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Call srand ONCE. \$\endgroup\$
    – Neil Kirk
    Commented Sep 26, 2015 at 23:32
  • \$\begingroup\$ The last bit of the value returned by rand() is not really that random rand()%2. You should check the C++11 random functions. \$\endgroup\$ Commented Sep 27, 2015 at 3:25

3 Answers 3

2
\$\begingroup\$

This is just going to be a simple and quick review.


using namespace std is bad practice due to naming conflicts that can easily occur.

Just use

std::what_you_need

while((playAgain == 'N') || (playAgain == 'n')){
    exit(EXIT_FAILURE);
}

Why is this a failure? It looks to me that the user wanted to exit and that the program deliberately exited without error. This should be

exit(EXIT_SUCCESS);

int randomNumber()
{
    int number;
    srand(time(0));

It is bad practice to be constantly seeding the random number generator. You should seed in once and only once. After that, you can use rand just as you have been.

You should put the srand call in main:

int main()
{
    srand(time(0))

int main()
{
    headsOrTails();
}

This looks a little ugly: if all main is doing is calling one other function only once, then it seems kind of wasteful to have that second function.

My first thought is that you could just move that entire function into main, but then you would have too much logic in main.

I suggest that you move the input part of headsOrTails into main, and then pass the input into the headsOrTails function.


You don't have any input validation. What if the user doesn't enter heads or tails?

You should do some checking of the user's input. If it's not anything desirable, then you simply tell the user, and then repeat asking them for input.


(playAgain == 'N') || (playAgain == 'n')

.

(prediction == "Heads") || (prediction == "heads")

Don't do checking on the different kinds of input the user can enter. Choose one string that they can enter and if they don't enter that, the input is invalid.

If you wanted to check all the possible inputs that the user could enter that is close to what you are looking for, you'd have a giant conditional.

\$\endgroup\$
0
4
\$\begingroup\$

Unnecessary loop

I noticed this code:

for(int i = 1; i < 2; i++)
{
    number = 1 + (rand()%2);
}

It's a loop that loops exactly once, and it doesn't even use i inside the loop. So you can just remove the loop part:

number = 1 + rand()%2;

Unecessary recursion

At the end of headsOrTails(), you have this code:

while((playAgain == 'Y') || (playAgain == 'y')){
    headsOrTails();
}
while((playAgain == 'N') || (playAgain == 'n')){
    exit(EXIT_FAILURE);
}
  1. Each while here could really be an if. Neither of the loops actually loop.
  2. It's not a good practice use recursion for no reason. Ideally, you would have a loop in main() that calls headsOrTails() as long as the user wants to keep playing. That way, you wouldn't need recursion. The only reason this even works instead of continuing indefinitely is because you call exit() eventually.
  3. Why EXIT_FALURE? That means your program will always return failure. I would use EXIT_SUCCESS instead.
\$\endgroup\$
0
\$\begingroup\$

Nobody here actually mentioned the bug in the code yet. You have this logic:

if((prediction == "Heads") || (prediction == "heads")){
    if(randomNumber() == 1){
        cout << "Heads, you are correct!\n" << endl;
    }
    else if(randomNumber() == 2){
        cout << "Tails, you were wrong.\n" << endl;
    }
}

Let's say the user picked "heads". We go into the first if conditional and call randomNumber(). If that returns 1, we print that the user won. Say it returns 2. Now we go into the else if conditional and call randomNumber() again. Now, half the time, it'll return 2 again and the program will look like it works. But the other half of the time, it'll return 1, and this conditional will also fail so that your code will print nothing.

You definitely want to only call randomNumber() once per prediction:

cin >> prediction;
int actual = randomNumber();

if (...) { ... }

Now, there's an awkward mismatch between what you're prompting the user to enter (a string like "heads") and what you're generating internally (a number that is 1 or 2). It'd be simpler to actually generate the same kind of thing internally:

std::string flip() {
    return (rand() % 2) ? "heads" : "tails";        
}

Now we can add a quick utility function to turn the user input into lower case:

std::string tolower(std::string input) {
    for (char& c : input) {
        c = tolower(c);
    }
    return input;
}

And then we can just compare directly:

cin >> prediction;
prediction = tolower(prediction);
std::string actual = flip();

if (prediction == "heads" || prediction == "tails") {
    if (prediction == actual) {
        // win!
    }
    else {
        // lose!
    }
}
else {
    // invalid input!
}

I find comparing like things easier to understand as far as the code goes.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.