3
\$\begingroup\$

I'm a beginner in programming and I would like to have feedback and tips to improve. He's a simple command line program. It's a Rock, Paper, Scissors game.

I guess I could use an enum for the "Attack", but I don't know how to use it with cout after that...

Thanks!

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>

using namespace std;

enum class Result
{
    win = 1,
    loss = -1,
    tie = 0
};

string ResultToString(Result result)
{
    switch (result)
    {
    case Result::win:
        return "WIN!";
    case Result::loss:
        return "LOSS!";
    default:
        return "TIE!";
    }
}

string DeterminePlay(char playAbbreviation)
{
    if (playAbbreviation == 'r')
    {
        return "Rock";
    }
    else if (playAbbreviation == 'p')
    {
        return "Paper";
    }
    else
    {
        return "Scissors";
    }
}

Result DetermineResult(char playerPlay, char computerPlay)
{
    if (playerPlay == 'r' && computerPlay == 's')
    {
        return Result::win;
    }
    else if (playerPlay == 'p' && computerPlay == 'r')
    {
        return Result::win;
    }
    else if (playerPlay == 's' && computerPlay == 'p')
    {
        return Result::win;
    }
    else if (playerPlay == 's' && computerPlay == 'r')
    {
        return Result::loss;
    }
    else if (playerPlay == 'r' && computerPlay == 'p')
    {
        return Result::loss;
    }
    else if (playerPlay == 'p' && computerPlay == 's')
    {
        return Result::loss;
    }
    else
    {
        return Result::tie;
    }
}

int GenerateRandomNumber(int min, int max)
{
    srand((int)time(0));
    int number{(rand() % max) + min};
    return number;
}

char GenerateComputerPlay()
{
    char computerPlay{};
    int number{GenerateRandomNumber(1, 3)};
    switch (number)
    {
    case 1:
        computerPlay = 'r';
        break;
    case 2:
        computerPlay = 'p';
        break;
    default:
        computerPlay = 's';
    }
    return computerPlay;
}

int main()
{
    setlocale(LC_ALL, "");
    char play{'y'};
    while (play == 'y')
    {
        char computerPlay{};
        char playerPlay{};

        cout << "--- Rock, Paper, Scissors ---" << endl << endl;

        while (playerPlay != 'r' && playerPlay != 'p' && playerPlay != 's')
        {
            cout << "Enter a choice : (R)ock, (P)aper, (S)cissors : ";
            cin >> playerPlay;
            playerPlay = tolower(playerPlay);
        }

        cout << endl << "You played " << DeterminePlay(playerPlay) << endl;

        computerPlay = GenerateComputerPlay();
        cout << "The computer played " << DeterminePlay(computerPlay) << endl;

        Result result{DetermineResult(playerPlay, computerPlay)};
        cout << "It's a " << ResultToString(result) << endl << endl;

        cout << "Play again?" << endl << "(Y)es" << endl << "(N)o" << endl;
        cin >> play;
        play = tolower(play);
    }
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ The program works as intended. There are small improvments you could do. For example the main loop could be a do...while as you will run it at least once, not that it matters much. You could do with some '\n' instead of so many endl. You only need to call srand once, in main, not in each GenerateRandomNumber call. You could use a enum or a const value called ROCK, SCISSORS, PAPER, instead of 'r', 's', 'p', for legibility. \$\endgroup\$ – mcabreb Feb 19 at 8:03
2
\$\begingroup\$

This is a good exercise in doing things in a more data-driven way. This will result in clearer separation of concepts (i.e., game logic) and allow for more customization easily.

So consider (skipping updating the main program):

#include <iostream>
#include <string>
#include <cstdlib>
#include <ctime>
#include <cctype>
#include <set>
#include <map>

enum Choice
{
    Rock = 1,
    Paper = 2,
    Scissors = 3
};

enum class Result
{
    Win,
    Loss,
    Tie
};

const std::set<std::pair<Choice, Choice> > wins =
{
    { Rock, Scissors },
    { Scissors, Paper},
    { Paper, Rock }
};

const std::map<Result, std::string> result_strings = 
{
    { Result::Win, "WIN!" },
    { Result::Loss, "LOSS!" },
    { Result::Tie, "TIE!" }
};

const std::map<char, std::string> play_abbreviations =
{
    { 'r', "Rock" },
    { 'p', "Paper" },
    { 's', "Scissors" }
};

std::string ResultToString(Result result)
{
    // TODO: Decide how to handle the case of input being unrecognized.
    return result_strings[result];
}

std::string DeterminePlay(char playAbbreviation)
{
    // TODO: Decide how to handle the case of input being unrecognized.
    return play_abbreviations[playAbbreviation];
}

Result DetermineResult(Choice playerPlay, Choice computerPlay)
{
    if (playerPlay == computerPlay)
    {
        return Result::Tie;
    }
    else
    {
        return wins.find(std::make_pair(playerPlay, computerPlay)) != wins.cend() ? Result::Win : Result::Loss;
    }
}

int GenerateRandomNumber(int min, int max)
{
    // TODO: Look at <random> to see how this is done better.
    srand(std::time(0));
    return (rand() % max) + min;
}

Choice GenerateComputerPlay()
{
    const int number = GenerateRandomNumber(1, 3);
    return static_cast<Choice>(number);
}

What we did here was that built the logic of who wins into a data structure, which is essentially a directed graph (more precisely, it's a directed 3-cycle). The winner determination now consist of checking whether the input is an arc in our graph; otherwise it's a loss if no such arc exists and a draw if the arc is a self-loop.

Notice also the possibilities you obtain by a having a result_strings: you could put in more "output texts" for a win for instance, so maybe sometimes you'd like to tell the user "WIN!", but other times you could print "GREAT JOB!" or "NICE!"; that'll end up a nightmare from a maintenance and logic point of view if the number of possible strings grows large.

\$\endgroup\$
1
\$\begingroup\$

Here's a small suggestion:

Your random number generation method is biased (taking the mod does not get you a uniform distribution). Also, you're seeding your random number every time you access it, while you can also just seed it once upon the start of the program (as @mcabreb mentioned). Additionally, lo and hi might be better parameter names for your GenerateRandomNumber function since min and max are actual function names in the STL. If you're using c++11, you can use the <random> header for generating uniform random numbers:

#include <random>
using namespace std;
std::default_random_engine generator;


int GenerateRandomNumber(int lo, int hi)
{
  std::uniform_int_distribution<int> distribution(lo, hi);
  return distribution(generator);
}



\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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