Rock-Paper-Scissors game

Question

This is the first piece of code I wrote on my own. I tried to use everything I know, like functions, loops and variables. Is it good or could it be simpler?

Please be hard as you can be with me because I feel my programming knowledge is quite poor and I end up using Google for solving my problems.

    // RPS.cpp : Defines the entry point for the console application.
//

#include "stdafx.h"
#include <iostream>
#include <ctime>
#include <cstdlib>

using namespace std;

int nPlayerScore=0;
int nComputerScore=0;
int nPlayerMove=0;
int nComputerMove=0;

void DisplayScore()
{
    cout << "Player: " << nPlayerScore << endl;
    cout << "Computer: " << nComputerScore << endl << endl;
}

int GetMove()
{
    cout << "Please choose a move: \n1.Rock\n2.Paper\n3.Scissors\n";
    int x;
    cin >> x;
    cout << endl;
    return x;
}

void CalculateResults()
{
    if (nPlayerMove==1) //player is Rock
    {
        if (nComputerMove==1) {cout << "player Rock vs computer Rock: IT'S A TIE!";}
        if (nComputerMove==2) {cout << "player Rock vs computer Paper: COMPUTER WON!"; nComputerScore=nComputerScore+1;}
        if (nComputerMove==3) {cout << "player Rock vs computer Scissors: PLAYER WON!"; nPlayerScore=nPlayerScore+1;}
    }
    if (nPlayerMove==2) //player is Paper
    {
        if (nComputerMove==1) {cout << "player Paper vs computer Rock: PLAYER WON!"; nPlayerScore=nPlayerScore+1;}
        if (nComputerMove==2) {cout << "player Paper vs computer Paper: IT'S A TIE!";}
        if (nComputerMove==3) {cout << "player Paper vs computer Scissors: COMPUTER WON!"; nComputerScore=nComputerScore+1;}
    }
    if (nPlayerMove==3) //player is Scissors
    {
        if (nComputerMove==1) {cout << "player Scissors vs computer Rock: COMPUTER WON!"; nComputerScore=nComputerScore+1;}
        if (nComputerMove==2) {cout << "player Scissors vs computer Paper: PLAYER WON!"; nPlayerScore=nPlayerScore+1;}
        if (nComputerMove==3) {cout << "player Scissors vs computer Scissors: IT'S A TIE!";}
    }

cout << endl << endl;
}

int main()
{
    do
    {
    cout << "Welcome to RPS 1.0a \tby GeTAFIX \n\n";
    DisplayScore();
    nPlayerMove=GetMove();
    srand((unsigned)time(0));
    nComputerMove=rand()%(3)+1;
    CalculateResults();
    //cout << "Press any key to continue...";
    system("pause");
    system("cls");
    } while (1==1);
}

Answer 1

First of all, stop using system("pause"); and system("cls");. Both of those are not portable, and are silly ways of achieving what you want. Take a look here for an actual solution.

Secondly, you are seeding the random number generator every iteration. This is not only unnecessary, it is harmful: if you do this in a program that calls rand more than once a second, you will get the same result each call. Even if you are only accessing it once a second (as you may be here), it is bad practice in any case, and can lead to a decrease in randomness.

Thirdly, I think you're getting a little far with splitting things into functions. It might make sense to split the move-reading part into a separate function if you're going to perform input validation, but I think it currently just complicates matters.

Your CalculateResults function is overcomplicated. If 0 is rock, 1 is paper, and 2 is scissors, and x and y are the choices of the players, you can tell who won with (3+x-y)%3. If the result is 1 then x has won; if it is 2, then y has won; otherwise (if it is 0) there has been a tie.

Further stylistic issues: you should fix your indentation, turn the do ... while loop into a while(true) or for(;;) loop, stop using using namespace std;, and disable precompiled headers (they offer no benefit for a single-file solution). You should also output '\n' instead of std::endl unless you explicitly want to flush the stream; an expression like std::cout << std::endl << std::endl simply doesn't make sense.

By the way, if you're using Google to solve your C++ problems, you're likely picking up a lot of information that's very wrong. Make sure you have a good book to learn from, such as Accelerated C++.

Your code could be simplified to this, although the behaviour isn't quite the same:

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

std::string numberToWeapon(int n) {
    switch (n) {
       case 0: return "rock";
       case 1: return "paper";
       case 2: return "scissors";
    }
    assert(!"Invalid parameter");
}

int main() {
    std::cout << "Rock-Paper-Scissors v1.0\n";

    int playerScore = 0;
    int ourScore = 0;
    std::srand(static_cast<unsigned int>(time(0)));

    while (true) {
         int playerMove;
         std::cout << "Please enter 1 for rock, 2 for paper, or 3 for scissors.\n";
         if (!(std::cin >> playerMove))
             return 0;
         --playerMove;
         if (playerMove < 0 || playerMove > 2) {
             std::cout << "Invalid input.\n";
             continue;
         }

         int ourMove = rand()%3;

         std::cout << "You played: " << numberToWeapon(playerMove) << ". ";
         std::cout << "We played: " << numberToWeapon(ourMove) << ".\n";
         switch ((3 + playerMove - ourMove) % 3) {
             case 0:
                 std::cout << "The game is a tie.\n";
                 break;
             case 1:
                 std::cout << "You won!\n";
                 ++playerScore;
                 break;
             case 2:
                 std::cout << "We won!\n";
                 ++ourScore;
                 break;
         }

         std::cout << "The score is currently:\n";
         std::cout << "You - " << playerScore << ".\n";
         std::cout << "We - " << ourScore << ".\n";
    }
}

This results in a larger main function, but no global variables. You can argue on whether that's any better or not: personally, I think it's clear as it is, and I don't see any duplication, but you may want to experiment.

If you are using C++11, you can get even more awesome syntax than the numberToWeapon function, namely:

std::array<4, std::string> weapons = {"rock", "paper", "scissors"};

And then use weapons.at(num) (or weapons[num] if you're sure you're in-bounds).

Answer 2

Agree with most of what Anton already mentioned:

In addition:

 using namespace std;

Stop using this.
It works out great for small programs but for large programs it becomes more of a problem. Getting in to the habit of not using it.

I would avoid over use of std::endl everywhere. What it does is place a newline character then flush the stream. If you have lots of output then it will cause the output to perform sub-optimally.

cout << "Player: " << nPlayerScore << endl;
cout << "Computer: " << nComputerScore << endl << endl;

In this case change two of the std::endl into "\n". Then use the last one to flush the output to the screen.

Good start but you should validate the input.
What If I typed Rock instead of 1. Then the stream gets broken and all further input is ignored. So validate your input and fix the stream if the bad bit is set.

int GetMove()
{
    cout << "Please choose a move: \n1.Rock\n2.Paper\n3.Scissors\n";
    int x;
    cin >> x;
    cout << endl;
    return x;
}

In this function:

void CalculateResults()

There are a lot of repeating strings. Anton suggested using functions to solve this. I think that is overkill for this situation and would just set up an array of strings.

static std::string  weapon[]    = { "Rock", "Paper", "Scissors" };
static std::string  result[3][] = {{ "IT'S A TIE!",   "COMPUTER WON!", "PLAYER WON!"  },
                                   { "PLAYER WON!",   "IT'S A TIE!",   "COMPUTER WON!"},
                                   { "COMPUTER WON!", "PLAYER WON!",   "IT'S A TIE!"  }
                                  };

Now you can easily display the weapons with:

std::cout << "player "   << weapon[nPlayerMove-1]
          << " Vs "
          << "computer " << weapon[nComputerMove-1]
          << "  "        << result[nPlayerMove-1][nComputerMove-1];

Lat of all:

cout << "Welcome to RPS 1.0a \tby GeTAFIX \n\n";

Here I would have used std::endl just to force a flush. Because the std::cin and std::cout are bound together with magic it does not really matter though.

Initializing the random number generator should be done once in the application. So do it just after main() starts. There is no need to cast the result to unsigned. A lot of people think 0 is good here I still like using the macro NULL (the problem with NULL is that on crappy compilers it is not defined correctly for C++). To me the NULL conveys more information in that it is supposed to be a pointer not the number 0. Though with C++11 I am trying to use nullptr.

Using this technique for generating a random value will get a lot of fanatics complaining.

nComputerMove=rand()%(3)+1;

Personally for this problem I think its fine. But a slightly better version would be:

 nComputerMove=(rand() / (RAND_MAX * 1.0)) * 3 + 1;

This makes the distribution slightly more even. Because 3 does not divide exactly into RAND_MAX your version has a slightly higher probability of being Rock 1/10922 than Scissors 1/10923 (assuming RAND_MAX of 32767). If you want to be really pedantic about it you would throw out any values greater than RAND_MAX/3*3. Then use mod 3.

Don't use this:

system("pause");
system("cls");