Rock-Paper-Scissors game

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
#include
#include

using namespace std;

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

void DisplayScore()
{
cout > 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;
C

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 
#include 
#include 
#include 
#include 

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 (time(0)));

    while (true) {
         int playerMove;
         std::cout > playerMove))
             return 0;
         --playerMove;
         if (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 weapons = {"rock", "paper", "scissors"};

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

StackExchange Code Review Q#9590, answer score: 5