Rock, paper, and scissors C++

Question

Could you give me your opinion about this simple rock, paper, and scissors game code and tell me tips to improve this code?

#include<iostream>
#include<cstdlib>
using namespace std;
enum element {rock = 1, scissors = 2, paper = 3};
enum result {lose = 4, draw = 5, win = 6};
short random (short from, short to)
{
 return rand() % (to - from + 1) + from;
}
short read (string msg)
{
 short num;
 cout << msg;
 cin >> num;
 return num;
}
result roundresult(element userchoice, element computerchoice)
{
 switch(userchoice)
 {
 case rock:
 {
 if (computerchoice == scissors)
 return win;
 else if (computerchoice == rock)
 return draw;
 else if (computerchoice == paper)
 return lose;
 }
 case scissors:
 {
 if(computerchoice == paper)
 return win;
 else if (computerchoice == scissors)
 return draw;
 else if (computerchoice == rock)
 return lose;
 }
 case paper:
 {
 if (computerchoice == rock)
 return win;
 else if (computerchoice == paper)
 return draw;
 else if (computerchoice == scissors)
 return lose;
 }
 }
}
short userscore(result currentresult, short userresult)
{
 if (currentresult == win)
 userresult++;
 return userresult;
}
short computerscore(result currentresult, short computerresult)
{
 if (currentresult == lose)
 computerresult++;
 return computerresult;
}
short drawscore(result currentresult, short drawtimes)
{
 if (currentresult == draw)
 drawtimes++;
 return drawtimes;
}
void printchoice(element userchoice, element computerchoice)
{
 switch (userchoice)
 {
 case(rock):
 {
 cout << "\nUser choice is rock\n";
 break;
 }
 case (paper):
 {
 cout << "\nUser choice is paper\n";
 break;
 }
 case (scissors):
 {
 cout << "\nUser choice is scissors\n";
 break;
 }
 }
 switch (computerchoice)
 {
 case(rock):
 {
 cout << "computer choice is rock\n";
 break;
 }
 case (paper):
 {
 cout << "computer choice is paper\n";
 break;
 }
 case (scissors):
 {
 cout << "computer choice is scissors\n";
 break;
 }
 }
}
void printresult(short length, element userchoice, element computerchoice, short& computerresult, short& userresult, short& drawtimes)
{
 computerresult = 0;
 userresult = 0;
 drawtimes = 0;
 for(short i = 0; i < length; i++)
 {
 cout << "Round [" << i + 1 << "] begins\n";
 userchoice = element(read("Enter your choice: \n1. Rock\n2. Scissors\n3. Paper\nYour choice is: "));
 cout << "-----------------------------" << endl;
 computerchoice = element(random(1, 3));
 printchoice(userchoice, computerchoice);
 if (userchoice == computerchoice)
 {
 cout << "Result is draw\n";
 cout << system("color 6f");
 }
 else if (roundresult(userchoice, computerchoice) == win)
 {
 cout << "User won\n";
 cout << system("color 2f");
 }
 else if (roundresult(userchoice, computerchoice) == lose)
 {
 cout << "Computer won\n";
 cout << system("color 4f");
 }
 userresult = userscore(roundresult(userchoice, computerchoice), userresult);
 computerresult = computerscore(roundresult(userchoice, computerchoice), computerresult);
 drawtimes = drawscore(roundresult(userchoice, computerchoice), drawtimes);
 cout << "\nresult is\nUser: " << userresult << "\nComputer: " << computerresult << endl;
 cout << "\n--------------------------------------------\n" << endl;
 }
}
void printfinalresult(short length, short computerresult, short userresult, short drawtimes)
{
 cout << "_______________G A M E O V E R________________\n";
 cout << "User winning times : " << userresult << endl;
 cout << "Computer winning times: " << computerresult << endl;
 cout << "Drawing times : " << drawtimes << endl;
 cout << "Final winner : ";
 if (userresult > computerresult)
 cout << "User" << endl;
 else if (userresult < computerresult)
 cout << "Computer" << endl;
 else
 cout << "No winner" << endl;
 cout << "Total rounds : " << length << endl;
 cout << "_______________________________________________" << endl << endl;
}
void singlegame()
{
 short computerresult, userresult, drawtimes = 0;
 element userchoice, computerchoice;
 srand((unsigned)time(NULL));
 short length = read("How many rounds? ");
 cout << endl << endl;
 printresult(length, userchoice, computerchoice, computerresult, userresult, drawtimes);
 printfinalresult(length, computerresult, userresult, drawtimes);
}
bool newgame()
{
 return read("Do you want to play again? [1] Yes, [0] No: ");
}
void startgame()
{
 bool askingnewgame = true;
 do
 {
 singlegame();
 askingnewgame = newgame();
 }
 while(askingnewgame);
}
int main ()
{
 startgame();
}

Answer 1

using namespace std;

This is quite harmful (particularly as you graduate to working on larger programs). It throws away useful information as to which identifiers you're really using, and opens up a whole world of potential collisions with your own names.

---

return rand() % (to - from + 1) + from;

Prefer to use the more modern facilities in <random> rather than std::rand(), which has no guarantees about how evenly distributed its results will be and is typically very poor, particularly in the low-order bits. You'll probably want to use a std::uniform_distribution for this.

---

cin >> num;
return num;

This completely fails to check whether the reading was successful. Never use a streamed-to variable before checking whether the stream state is good.

---

 cout << endl << endl;

Why are we flushing std::cout twice in quick succession? I recommend never using std::endl - instead, use std::flush where it's really needed:

 std::cout << "\n\n";

 std::cout << msg << std::flush;
 cin >> num;

• Piotr Siupa: You don't need to flush cout before cin. std::cin is by default tied to std::cout which mean it will flush it automatically. stackoverflow.com/a/2704757/3052438
• Piotr Siupa: I'd say flushing is useful only if you perform some long running calculation after and you want to make sure that the current output is displayed to the user immediately. Unless you explicitly untied std::cerr and std::cin from std::cout, everything else should behave correctly.
• Toby Speight: That's true - this program doesn't have any real need for flushing.

Answer 2

Verify user inputs

Rule #1 in application development: Never trust user input!

For example:

userchoice = element(read("Enter your choice: \n1. Rock\n2. Scissors\n3. Paper\nYour choice is: "));

If the user enters "4", your application would run the logic, and may end up in a bad state or return unexpected results. You're advised to make sure the user really enters what you expect and return a useful error message.

Similarly, if the user answers the question "How many rounds? with "50000", your program will probably loop indefinitely.

To write robust code, always also think about what could go wrong, always expect the unexpected. This even applies to internal/private functions because after later changes they may be called in unexpected ways.

No-return branches

Not all branches of roundresult return a result. For example, the call roundresult(4, 5) would be a valid call, but the result is undefined (it probably returns 0, which is not a valid instance of result). It is generally good practice to make sure all possible execution branches of non-void functions eventually return something. This makes your code more obvious and helps to avoid unexpected results.

Don't repeat yourself

You can optimize your code here and there to avoid repetition, e.g., the "User choice is" is redundant in the printchoice method.

Try to use curly brackets

I would recommend to always use curly brackets, even if you don't have to use them, so for example:

if (userresult > computerresult) {
 cout << "User" << endl;
}

This is certainly a matter of taste, but when changing the code not carefully enough, it may lead to bugs.

Readability

Readability is important in a good code base to support future developers (including your future self) understanding the code. This already starts with proper word separation, in C/C++ you normally either use camel case (e.g., printChoice) or underscores (e.g., print_choice).

You can also use capitalization to ease the distinction between types and variables/methods/functions, e.g., enum Result vs. result = 1.

Lastly, I recommend to use clear verbs for methods/functions, e.g., rename random to get_random_number and computerscore to update_computer_score.