Rock Paper Scissors C# game with rounds input

Question

I have a Rock Paper Scissors project in C# and I was wondering if I could get some feedback on how to improve my code. I ask the user how many rounds the user wants to play. The maximum number of rounds allowed is 10, anything else besides a number 1-10, will exit the program. I would also like to know if I could use a for loop to count the number of rounds until it reaches the rounds input and when user wants to play again, the count of rounds should reset to 0. Not sure if int countRounds = 0 is actually required for this purpose.

using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace RockPaperScissors
{
 class Program
 {
 public static int roundsInput;
 public static bool inPlay = true;
 public static string player;
 static void Main(string[] args)
 {
 PlayGame();
 }
 public static void PlayGame()
 {
 string numOfRounds;
 string selectMode;
 int inputMode;
 int computer;
 bool roundsInRange = false;
 int countRounds = 0;
 int countWins = 0;
 int countLoses = 0;
 int countTies = 0;
 IList rpsList = new ArrayList() { "rock", "paper", "scissors" };
 Random r = new Random();
 do
 {
 if (!roundsInRange)
 {
 Console.WriteLine("Rock Paper Scissors [Max rounds is 10]");
 Console.WriteLine("Enter the number of rounds: ");
 numOfRounds = Console.ReadLine();
 if (int.TryParse(numOfRounds, out roundsInput))
 {
 if (roundsInput >= 1 && roundsInput <= 10)
 {
 roundsInRange = true;
 }
 else if (roundsInput < 1 || roundsInput > 10)
 {
 Console.WriteLine("Number out of range");
 break;
 }
 else
 {
 Console.WriteLine("Not valid");
 break;
 }
 }
 }
 if (roundsInRange)
 {
 Console.WriteLine("Choose Rock, Paper, or Scissors: ");
 Console.WriteLine("1 - Rock");
 Console.WriteLine("2 - Paper");
 Console.WriteLine("3 - Scissors");
 selectMode = Console.ReadLine();
 computer = r.Next(0, rpsList.Count);
 if (int.TryParse(selectMode, out inputMode))
 {
 switch (inputMode)
 {
 case 1:
 player = "rock";
 break;
 case 2:
 player = "paper";
 break;
 case 3:
 player = "scissors";
 break;
 default:
 Console.WriteLine("Invalid input");
 break;
 }
 }
 if (player == rpsList[computer].ToString())
 countTies++;
 else if (player == "rock" && rpsList[computer].Equals("scissors")
 || player == "paper" && rpsList[computer].Equals("rock")
 || player == "scissors" && rpsList[computer].Equals("paper"))
 countWins++;
 else countLoses++;
 countRounds++;
 Console.WriteLine($"Opponent Answer: {rpsList[computer]}");
 Console.WriteLine($"Ties: {countTies} Wins: {countWins} Loses: {countLoses}");
 Reset(countRounds, countWins, countLoses, countTies);
 }
 } while (inPlay);
 }
 public static void Reset(int rounds, int wins, int loses, int ties)
 {
 if (rounds == roundsInput)
 {
 string playAgain;
 if (wins > loses)
 {
 Console.WriteLine("You win!");
 }
 if (wins == loses)
 {
 Console.WriteLine("Tie!");
 }
 if (wins < loses)
 {
 Console.WriteLine("You Lose!");
 }
 Console.WriteLine("Would you like to play again? Type: y/n");
 playAgain = Console.ReadLine();
 if (playAgain == "y")
 {
 PlayGame();
 }
 else if (playAgain == "n")
 {
 Console.WriteLine("Thanks for playing!");
 inPlay = false;
 }
 else
 {
 Console.WriteLine("Invalid input");
 inPlay = false;
 }
 }
 }
 }
}

Answer 1

Single responsibility

PlayGame is asking the user how many rounds to play, and it handles each round - all in the same loop. It also handles 'low-level' input validation, and it attempts to 'reset' the game after every round. That's too many different responsibilities for a single method.

Reset doesn't reset anything, but it does show the results of the current game and it starts a new game - but only if the current game is in the last round.

That's all fairly confusing. It's better when classes and methods have a single, clearly defined responsibility, for example:

• int GetIntInput(string prompt, int minValue, int maxValue) for input handling. This would keep asking until the user has given valid input. A boolean variant (for yes/no input) would also be useful.
• void PlayGame(int rounds) would play a single game. Internally, I would indeed use a for loop: for (int round = 0; round < rounds; round++) { ... }. After that loop, it would display the results of the game and then return.
• void Main(string[] args) would ask the user how many rounds to play, and then call PlayGame. Afterwards, it can ask the user if they want to play again. This avoids the recursion between your PlayGame and Reset methods, which can lead to a stack overflow when playing many games in succession.
• Methods like bool Ties(string myChoice, string opponentChoice) and bool Beats(string myChoice, string opponentChoice) would let you simplify the round-handling logic a bit.

Problems

• In the roundsInput validation, the final else block will never be executed. It should be moved out of the if (int.TryParse(numOfRounds, out roundsInput)) body.
• else if (roundsInput < 1 || roundsInput > 10) can be simplified to just else. The check above already takes care of the range check.
• During gameplay, chosing anything other than 1, 2 or 3 causes the game to use your last choice again. It would be better to continue asking for valid input before proceeding.

Other notes

• It's better to declare variables as close to where they will be used as possible, and to initialize them immediately, instead of declaring them up-front at the start of a method.
• ArrayList is an old type that was created before C# had generics. Nowadays you should use List<T> instead. With rpsList as a List<string>, the compiler knows that rpsList[index] will return a string, so you don't need those ToString() calls, and you can use the == operator for string comparisons instead of Equals.
• The strings "rock", "paper" and "scissors" are repeated several times throughout the code. It's easy to introduce a bug by making a typo. Use constants instead: public const string Rock = "rock";. Or rather, use an enum: public enum Choice { Rock, Paper, Scissor }. This makes it clear that a Choice can only be one of these 3 values, whereas a string can hold any possible string value.
• out variables can be declared inline. Also note that the results of Console.ReadLine() are only used once. This means that input parsing can be simplified to if (int.TryParse(Console.ReadLine(), out int rounds)).
• Instead of repeating rpsList[computer], you might as well store the choice directly (and rename that variable to something more descriptive): opponentChoice = rpsList[r.Next(0, rpsList.Count)];.
• Instead of if (condition) { ... }, it's better to use an early-out return to reduce nesting: if (!condition) return; ....
• Static fields may seem like a convenient way to communicate between methods, but they lead to dependencies between methods that are hard to track, ultimately leading to code that is difficult to manage. It's better to communicate via parameters and return values only. In this case, roundsInput and player should be local variables in PlayGame, and inPlay and Reset are better solved in a different way (by letting Main or another top-level method handle the restarting).
• if (wins > loses) .. if (wins == loses) .. if (wins < loses) .. can be simplified to if (wins > loses) .. else if (wins == loses) .. else ... This also ensures that, even when a condition is modified, you never get more then one message.

• Alejandro H: Excellent, I learned a lot, I ended up removing Reset and ended up asking the number of rounds in static main and then passing rounds to 'PlayGame(int rounds)'. Also you mentioned I should use 'List<string>', but I think I used 'IList<string>'. I did that and I wanted to know if using this actually lets me access the list as an index and lets say I decided to use string[] rpsList = ... , would this not let me access the list as an index would it?
• Pieter Witvoet: IList<T> contains an indexer, and both List<T> and T[] implement IList<T>, so a variable of type IList<string> can be assigned either a list or an array of strings. Indexing will work fine in both cases. There's no benefit to making rpsList an IList<string> here though - that's more useful for method parameters, so methods aren't limited to a single type only. In this case I would use var rpsList = ..., so it'll have the same type as whatever you're assigning to it, without having to write the type name twice.