RPSLS Game in C#

Question

I went with what I know and can use well, not what I know and can't figure the syntax out to make it look good.

So please enjoy the code and prepare to school me (probably in the basics) in C#

public static void Main (string[] args)
{
 /* Here are your rules: 
 "Scissors cuts paper, 
 paper covers rock, 
 rock crushes lizard, 
 lizard poisons Spock, 
 Spock smashes scissors, 
 scissors decapitate lizard, 
 lizard eats paper, 
 paper disproves Spock, 
 Spock vaporizes rock. 
 And as it always has, rock crushes scissors." 
 -- Dr. Sheldon Cooper */
 List<string> listOfGestures = new List<string>();
 listOfGestures.Add("rock");
 listOfGestures.Add ("paper");
 listOfGestures.Add ("scissors");
 listOfGestures.Add ("lizard");
 listOfGestures.Add ("spock");
 int win = 0;
 int lose = 0;
 int tie = 0;
 var newGame = true;
 while (newGame)
 {
 var playerGesture = "";
 var validInput = false;
 while (!validInput)
 {
 var playerChoice = -1;
 Console.WriteLine("Please choose your Gesture ");
 gameSetup(listOfGestures);
 try
 {
 playerChoice = Convert.ToInt32 (Console.ReadLine());
 }
 catch (FormatException e)
 {
 validInput = false;
 }
 if (playerChoice > 0 && playerChoice <= listOfGestures.Count)
 {
 playerGesture = listOfGestures[playerChoice - 1];
 validInput = true;
 }
 else
 {
 validInput = false;
 }
 }
 var computerPlay = ComputerGesture(listOfGestures);
 Console.WriteLine ("Computer: " + computerPlay);
 Console.WriteLine ("your Gesture: " + playerGesture);
 if (playerGesture == computerPlay)
 {
 tie++;
 Console.WriteLine ("you have tied with the computer");
 Console.WriteLine ("Computer: " + computerPlay);
 Console.WriteLine ("your Gesture: " + playerGesture);
 }
 else
 {
 if (playerGesture == "rock")
 {
 if (computerPlay == "lizard" || computerPlay == "scissors")
 {
 Console.WriteLine ("You Win, " + playerGesture + " Crushes " + computerPlay);
 win++;
 }
 else if (computerPlay == "paper")
 {
 Console.WriteLine ("You Lose, Paper Covers Rock");
 lose++;
 }
 else if (computerPlay == "spock")
 {
 Console.WriteLine ("You Lose, Spock Vaporizes Rock");
 lose++;
 }
 } 
 else if (playerGesture == "paper")
 {
 if (computerPlay == "spock")
 {
 Console.WriteLine ("You Win, Paper Disproves Spock");
 win++;
 } 
 else if (computerPlay == "rock")
 {
 Console.WriteLine ("You Win, Paper Covers Rock");
 win++;
 }
 else if (computerPlay == "lizard")
 {
 Console.WriteLine ("You Lose, Lizard Eats Paper");
 lose++;
 }
 else if (computerPlay == "scissors")
 {
 Console.WriteLine ("You Lose, Scissors Cut Paper");
 lose++;
 }
 }
 else if (playerGesture == "scissors")
 {
 if (computerPlay == "paper")
 {
 Console.WriteLine ("You Win, Scissors Cut Paper");
 win++;
 }
 else if (computerPlay == "lizard")
 {
 Console.WriteLine ("You Win, Scissors Decapitate Lizard");
 win++;
 }
 else if (computerPlay == "rock")
 {
 Console.WriteLine ("You Lose, Rock Crushes Scissors");
 lose++;
 }
 else if (computerPlay == "spock")
 {
 Console.WriteLine ("You Lose, Spock Smashes Scissors");
 lose++;
 }
 }
 else if ( playerGesture == "lizard")
 {
 if (computerPlay == "paper")
 {
 Console.WriteLine ("You Win, Lizard Eats Paper");
 win++;
 }
 else if (computerPlay == "spock")
 {
 Console.WriteLine ("You Win, Lizard Poisons Spock");
 win++;
 }
 else if (computerPlay == "scissors")
 {
 Console.WriteLine ("You Lose, Scissors Decapitates Lizard");
 lose++;
 }
 else if (computerPlay == "rock")
 {
 Console.WriteLine ("You Lose, Rock Crushes Lizard");
 lose++;
 }
 }
 else if (playerGesture == "spock")
 {
 if (computerPlay == "rock")
 {
 Console.WriteLine("You Win, Spock Vaporizes Rock");
 win++;
 }
 else if (computerPlay == "scissors")
 {
 Console.WriteLine ("You Win, Spock Smashes Scissors");
 win++;
 }
 else if (computerPlay == "paper")
 {
 Console.WriteLine ("You Lose, Paper Disproves Spock");
 lose++;
 }
 else if (computerPlay == "lizard")
 {
 Console.WriteLine("You Lose, Lizard Poisons Spock");
 lose++;
 }
 }
 }
 Console.WriteLine ("Your Score is (W:L:T:) : {0}:{1}:{2}", win, lose, tie);
 Console.WriteLine ("Again? Type 'n' to leave game.");
 if (Convert.ToString (Console.ReadLine ().ToLower ()) == "n")
 {
 Console.WriteLine ("would you like to reset your Score?");
 if (Convert.ToString (Console.ReadLine ().ToLower ()) == "y")
 {
 win = 0;
 lose = 0;
 tie = 0;
 }
 Console.WriteLine ("would you like to play another game?");
 Console.WriteLine ("if you type 'n' the game will end.");
 if (Convert.ToString(Console.ReadLine().ToLower()) == "n")
 {
 newGame = false;
 }
 }
 }
 Console.WriteLine("Goodbye");
 Console.ReadLine ();
}

Helper Methods that I used. I could probably use a couple more.

public static void gameSetup (List<string> List)
{
 for (int i=0; i<List.Count; i++) {
 Console.WriteLine ((i+1) + ": " + List[i]);
 } 
}

With Random here I am not sure that I am using it correctly or if Random should be a static variable in the class scope and I should just be calling rand.next() everytime that I need to use it.

public static string ComputerGesture (List<string> options)
{
 Random rand = new Random();
 return options[rand.Next(0,options.Count)];
}

• abuzittin gillifirca: I actually like gestures be just a list of strings. Later if/else spaghetti kills the appetite though. ChrisWue is right in pointing out that you are using magic constants all over the place. The payoff logic is a static data structure also, which some other RPSLS implementors here got; and can be factored out just like you did for the gestures, which some other implementors missed.

Answer 1

1. Some minor syntax detail: C# has collection initializers so one can write:

   List<string> listOfGestures = new List<string> {
    "rock", "paper", "scissors", "lizard", "spock" 
   }
   

2. You use magic strings for the gestures all over the place. This can pose a problem if you want to change them. You could do a find and replace but this could inadvertently change a part of a method or variable name which happens to include "rock" if you are not careful.

   In other solutions to the problem posted before enums were used instead. If you insist on using strings then make them named constants and use those instead

   const string Rock = "rock";
   const string Paper = "paper";
   ... etc.
   

3. The main problem is that the entire implementation is basically just one big blob of code in the main method. Logic and input/output are totally intertwined. Assume you have to write an automated test case for this - it will drive you insane.

   If I were to encounter code like this in production I would very likely kill it and rewrite it - the problem is fairly simple and given it's untestable there aren't any unit tests anyway. If the code were too much to rewrite then I'd start refactoring it. Possible steps:

   1. Create a Game class which effectively encapsulates the main while loop up until the line Console.WriteLine ("Your Score is (W:L:T:) : {0}:{1}:{2}", win, lose, tie);

      • Dump everything into one method into that class for starters.
      • The Game class should also keep track of the score.
      • Override ToString to print out the current scores.
      • Move the "ask user if he wants to play another round" into a method on the Game class

      After that the Main should look somewhat like this:

      var game = new Game();
      var continuePlaying = true;
      while (continuePlaying)
      {
       game.Play();
       Console.WriteLine(game.ToString());
       continuePlaying = game.ShouldContinue();
      }
      

   2. Next step would be to start refactoring the Game class and the methods within.

      • Move the decision logic into it's own internal private method to reduce the bulk of the Play() method.

      • Define an interface for getting user input and another one for writing output. Pass these interfaces into the Game constructor. Provide a console implementation for both interfaces. Change all the input and output to use the interfaces rather than the console.

      • Now Game is decoupled from the specific input and output methods and you can start writing unit tests for the Play() and the ShouldContinue() methods. Once you have done that you can continue refactoring those methods. And because you have just created unit tests you should be able to detect if you create any regressions (introducing bugs) while doing so.

      • Wash, Rinse, Repeat

• Malachi: thank you for giving me the next couple of steps to take, this helps me with the learning process. I won't be able to put up new revised code for a week or so, but you can bet that I will work on this game in a week or so. Thank you
• svick: Console.WriteLine(game.ToString()); This can be simplified to just Console.WriteLine(game);.
• svick: Also, when you have strings all over the place, refactoring is an issue, but I think much bigger problem are typos: it's very easy to make a single typo somewhere and it's not trivial to discover bugs like that.

Answer 2

1. An enum will serve you much better than a list of strings.

   enum Gesture
   {
    Rock = 1,
    Paper = 2,
    Scissors = 3,
    Spock = 4,
    Lizard = 5
   }
   

2. A naming convention. If a variable is a counter and its name describes what is being counted, it should then be plural.

   int wins = 0;
   int loses = 0;
   int ties = 0;
   

3. And now at this point, I don't want to just correct small bits of this code, but restructure it to use some OOP. But I won't, just know that next time you make a game, OOP is much better, modularization is the keyword right there (if it even is a word).

4. The Method Name gameSetup should start with a capital letter. Especially because it is public, if it was private you could make it lower-case, though I would still not recommend it.

5. Inside GameSetup you really aren't doing a lot of logic that you would normally want removed from the pretty business logic. On top of that, you are only calling this function in one place in your code, so it benefits you about.... not at all. And now that I've taken away your gesture list I will replace the code that you will need here for that.

   Using foreach:

   foreach (Gesture gesture in (Gesture[])Enum.GetValues(typeof(Gesture)))
   {
    Console.WriteLine((int)gesture + Enum.GetName(typeof(Gesture), gesture));
   }
   

   Using for:

   for (int i = 1; i <= 5; i++)
   {
    Console.WriteLine(i + Enum.GetName(typeof(Gesture), i));
   }
   

   While either of the above loops will work, I would recommend only using the first one. As the second one only works aslong as you don't change the enum at all, which in any other situation would not work out so well.

6. You mentioned that you weren't sure about how you were using the Random class. You said or if Random should be a static variable in the class scope. Yes, you should move rand to the class scope. However that is not 100% necessary. I want to say though, that as you are declaring rand in a static method, it is a static variable.. infact everything you are doing here is static. If you had gone the way of OOP you would have had some non-static stuff, but.. you didn't.

7. ComputerGesture is another example of a method which is only called once and does very little code. This would have been better suited to stay with the rest of the main logic and just had a comment placed above it saying, computer gesture. Also a code change, which is just something I like to do.. replace the contents of that method with. Note... I'm also renaming this to GetComputerGesture(), because.... it gets the computer gesture

   return options[new Random().Next(0, options.Count)];
   

   however, because I changed your string array to enums, this is what it would have to look like

   public static Gesture GetComputerGesture()
   {
    var vals = Enum.GetValues(typeof(Gesture));
    return (Gesture)vals.GetValue(new Random().Next(vals.Length));
   }
   

   ---

   Now lets start on your main loop.

8. This isn't super important, but because you do know that you will be playing atleast one game, you could make your main while loop, a do while. the same goes for your next loop checking to see if the user input is valid.

9. Now user input is something that I personally would separate out into a method. It is a large chunk of code, and though in your case it is only being called once, it is something that you may want to re-use if you add a second user player. So lets do that...

   Here is a method I wrote to get an integer value from the user, and reject that value if it did not fall between the given range. Notice how I use int.TryPase() instead of attempting to parse the integer in a try block. I did this because, it is both nicer code, and is more efficient. Exception handling is a costly run-time operation, and relying on an exception to be thrown should be avoided if at all possible. If you have not seen the out keyword before, this would be a good time to do some research. As you can see when I call int.TryParse() it takes an out parameter. These are the C# version of pointers (While pointers do exist... don't use them as long as you don't have to). So instead of TryParse returning the parsed number, it returns whether or not it succeeded, and sends out the parsed value in its out parameter, in this case g.

   I should also point out that I used default values in the parameters, this means now whenever you use this method, you can actually not send in parameters and they will default to the specified values. If you wanted to only change the value of prompt when calling this method you would just say this GetIntInRange(prompt: "Hey this is my new prompt"); and the others will default.

   public static int GetIntInRange(int min = int.MinValue, int max = int.MaxValue, string prompt = "Please enter an Integer: ")
   {
    int g;
    do
    {
    Console.Write(prompt);
    if (int.TryParse(Console.ReadLine(), out g))
    {
    if (g >= min && g <= max)
    return g;
    Console.WriteLine("You entered {0}, but the input must be in the range of ({1} - {2}. Please try again...", g, min, max);
    }
    else
    Console.WriteLine("That is not a real number. Please try again...");
    } while (true);
   }
   

10. I know earlier I said that it was fairly pointless to have ComputerGesture be its own method, because it was doing very limited things and was only called once. I will say however because I do expect you to change this program to be Object Oriented, or at-least make your next game as such, that an operation like Computer or Player Gesture should be modularized. Its just in your main loop it didn't make a difference. That being said, lets make a super short method called GetPlayerGesture! We can use that GetIntInRange Method we just wrote. Also at this point, I realize we have had to use the list of Gesture enums a lot, so I just decided to add this private static member. So that we don't have to keep creating the list on the fly.

    private static Gesture[] _gestures = (Gesture[])Enum.GetValues(typeof(Gesture));
    

    And now for the GetPlayerGesture Method.... 1 line :D

    public static Gesture GetPlayerGesture()
    {
     return (Gesture)GetIntInRange((int)_gestures.First(), (int)_gestures.Last(), "Please choose your Gesture: ");
    }
    

11. Progress update on the Main Method (comments removed to not take up too much space)

    public static void Main(string[] args)
    {
     int wins = 0;
     int loses = 0;
     int ties = 0;
     var newGame = true;
     do
     {
     GameSetup();
     Gesture playerGesture = GetPlayerGesture();
     Gesture computerGesture = GetComputerGesture();
     Console.WriteLine("Computer: " + computerGesture);
     Console.WriteLine("Your Gesture: " + playerGesture);
     //The rest of the program here
    

12. The one thing that is absolutely needed in here, is to move the code that calculates who won to another method, this is a case where code is just too big to chill with your main logic. If you wanted to, you could just put #region tags around this chunk, and call it good too, but this code should also be made more generic incase you wanted to use 2 players instead of just one and a computer, or have 2 computers duke it out 1,000,000,000 times in a couple seconds. I also feel this code could be written more efficiently... lets try.

    In the following method I converted each Gesture to an int, which you will notice were defined when we created the enum at the top. We can also see that every gesture loses to a number which is one higher than itself, but beats any number 2 higher than itself, and this pattern continues to alternate. So if we add up the numbers and the number is odd, that means the player with the higher number wins. At this point there are quite a few ways we could go about this.

    This little bit of code adds player1 and player2's enum values together, and gets the remainder when dividing by 2, if there is no remainder, then the sum of these two values is even (p1 + p2) % 2 == 0.

    Now we only want to say that player 1 won if player1 chose an even number and the sum of the choice was even, or if the opposite of both of these are true, The logical result we are looking for is called XNOR, which is simply bool == bool. So our Method would look like this.

    public static int WhoWon(Gesture player1, Gesture player2)
    {
     if (player1 == player2) return 0;
     int p1 = (int)player1;
     int p2 = (int)player2;
     bool isEven = (p1 + p2) % 2 == 0;
     bool player1IsHigher = player1 >player2;
     if (player1IsHigher == isEven)
     return 1;
     else 
     return 2;
    }
    

    But, because we like to be concise and complex as programmers, lets collapse some of this together and use a couple inline ORs

    public static int WhoWon(Gesture player1, Gesture player2)
    {
     return (player1 == player2) ? 0 : (((int)player1 % 2 == 0) == (((int)player1 + (int)player2) % 2 == 0)) ? 1 : 2;
    }
    

    Now we have a simple little method that returns 0 if there is a tie, or 1,2 depending on who the winner is.

13. We need the customized messages revamped now that we changed how we decide the winner. For this I have created a Dictionary, string> with all the possible actions which will happen when a specific Gesture wins over another. I used Tuple objects as keys and strings for values. Instead of only storing the actions, I could have stored the entire response string... but that would have been more typing for me. So here is the dictionary.

    private static Dictionary<Tuple<int, int>, string> _actions = new Dictionary<Tuple<int, int>, string>()
    {
     {Tuple.Create<int,int>(1,3), "Crushes"},
     {Tuple.Create<int,int>(1,5), "Crushes"},
     {Tuple.Create<int,int>(2,1), "Covers"},
     {Tuple.Create<int,int>(2,4), "Disproves"},
     {Tuple.Create<int,int>(3,2), "Cuts"},
     {Tuple.Create<int,int>(3,5), "Decapitates"},
     {Tuple.Create<int,int>(4,3), "Smashes"},
     {Tuple.Create<int,int>(4,1), "Vaporizes"},
     {Tuple.Create<int,int>(5,2), "Eats"},
     {Tuple.Create<int,int>(5,4), "Poisons"}
    };
    

    And here is all you need to retrieve the reason why someone won or lost.

    public static string GetReason(Gesture winner, Gesture loser)
    {
     return winner + " " + ACTIONS[Tuple.Create((int)winner, (int)loser)] + " " + loser;
    }
    

14. I would do the following differently if this was Object Oriented. Just so you know... if you were doing this OO, you would have a Player which remember how many wins and losses that player had. So for this program, we are assuming that wins, loses, ties are playerWins, playerLoses, playerTies.

    Here, I've written a Method to simply get Y or N input and not even recognize anything else.

    public static bool GetYN(string prompt = "(y/n): ")
    {
     do
     {
     Console.Write(prompt);
     switch (Console.ReadKey(true).Key)
     {
     case ConsoleKey.Y: Console.Write("Y\n"); return true;
     case ConsoleKey.N: Console.Write("N\n"); return false;
     }
     } while (true);
    }
    

    Using these last two methods I've refactored the rest of your logic using them, and this is what I got in the end

     switch (WhoWon(playerGesture, computerGesture))
     {
     case 0: ties++; Console.WriteLine("You have tied with the the computer."); break;
     case 1: wins++; Console.WriteLine("You win, " + GetReason(playerGesture, computerGesture)); break;
     case 2: loses++; Console.WriteLine("You lose, " + GetReason(computerGesture, playerGesture)); break;
     }
     Console.WriteLine("Your Score is (W:L:T:) : {0}:{1}:{2}", wins, loses, ties);
     Console.Write("Would you like to play again? ");
     if(!GetYN())
     {
     Console.WriteLine("would you like to reset your Score?");
     if (GetYN())
     {
     wins = loses = ties = 0;
     }
     Console.Write("Would you like to play again? ");
     newGame = GetYN();
     }
     Console.WriteLine();
     } while (newGame);
     Console.WriteLine("Goodbye\nPress any key to close...");
     Console.ReadKey(true);
    }
    

In-case I forgot anything, or you just want to grab the sum of the changes, here is the whole dump.

enum Gesture
{
 Rock = 1,
 Paper = 2,
 Scissors = 3,
 Spock = 4,
 Lizard = 5
}
class Program
{
 private static Gesture[] _gestures = (Gesture[])Enum.GetValues(typeof(Gesture));
 private static Dictionary<Tuple<int, int>, string> _actions = new Dictionary<Tuple<int, int>, string>()
 {
 {Tuple.Create<int,int>(1,3), "Crushes"},
 {Tuple.Create<int,int>(1,5), "Crushes"},
 {Tuple.Create<int,int>(2,1), "Covers"},
 {Tuple.Create<int,int>(2,4), "Disproves"},
 {Tuple.Create<int,int>(3,2), "Cuts"},
 {Tuple.Create<int,int>(3,5), "Decapitates"},
 {Tuple.Create<int,int>(4,3), "Smashes"},
 {Tuple.Create<int,int>(4,1), "Vaporizes"},
 {Tuple.Create<int,int>(5,2), "Eats"},
 {Tuple.Create<int,int>(5,4), "Poisons"}
 };
 public static void Main(string[] args)
 {
 /* Here are your rules: 
 "Scissors cuts paper, 
 paper covers rock, 
 rock crushes lizard, 
 lizard poisons Spock, 
 Spock smashes scissors, 
 scissors decapitate lizard, 
 lizard eats paper, 
 paper disproves Spock, 
 Spock vaporizes rock. 
 And as it always has, rock crushes scissors." 
 -- Dr. Sheldon Cooper */
 int wins = 0;
 int loses = 0;
 int ties = 0;
 var newGame = true;
 do
 {
 GameSetup();
 Gesture playerGesture = GetPlayerGesture();
 Gesture computerGesture = GetComputerGesture();
 Console.WriteLine("Computer: " + computerGesture);
 Console.WriteLine("Your Gesture: " + playerGesture);
 switch (WhoWon(playerGesture, computerGesture))
 {
 case 0: ties++; Console.WriteLine("You have tied with the the computer."); break;
 case 1: wins++; Console.WriteLine("You win, " + GetReason(playerGesture, computerGesture)); break;
 case 2: loses++; Console.WriteLine("You lose, " + GetReason(computerGesture, playerGesture)); break;
 }
 Console.WriteLine("Your Score is (W:L:T:) : {0}:{1}:{2}", wins, loses, ties);
 Console.Write("Would you like to play again? ");
 if (!GetYN())
 {
 Console.WriteLine("Would you like to reset your Score?");
 if (newGame = GetYN())
 {
 wins = loses = ties = 0;
 }
 if (!newGame)
 break;
 Console.Write("Would you like to play again? ");
 newGame = GetYN();
 }
 Console.WriteLine();
 } while (newGame);
 Console.WriteLine("Goodbye\nPress any key to close...");
 Console.ReadKey(true);
 }
 public static int WhoWon(Gesture player1, Gesture player2)
 {
 return (player1 == player2) ? 0 : (((int)player1 % 2 == 0) == (((int)player1 + (int)player2) % 2 == 0)) ? 1 : 2;
 }
 public static int GetIntInRange(int min = int.MinValue, int max = int.MaxValue, string prompt = "Please enter an Integer: ")
 {
 int g;
 do
 {
 Console.Write(prompt);
 if (int.TryParse(Console.ReadLine(), out g))
 {
 if (g >= min && g <= max)
 return g;
 Console.WriteLine("You entered {0}, but the input must be in the range of ({1} - {2}. Please try again...", g, min, max);
 }
 else
 Console.WriteLine("That is not a real number. Please try again...");
 } while (true);
 }
 public static bool GetYN(string prompt = "(y/n): ")
 {
 do
 {
 Console.Write(prompt);
 switch (Console.ReadKey(true).Key)
 {
 case ConsoleKey.Y: Console.Write("Y\n"); return true;
 case ConsoleKey.N: Console.Write("N\n"); return false;
 }
 } while (true);
 }
 public static void GameSetup()
 {
 foreach (Gesture gesture in _gestures)
 {
 Console.WriteLine((int)gesture + ": " + Enum.GetName(typeof(Gesture), gesture));
 }
 }
 public static Gesture GetComputerGesture()
 {
 return (Gesture)_gestures.GetValue(new Random().Next(_gestures.Length));
 }
 public static Gesture GetPlayerGesture()
 {
 return (Gesture)GetIntInRange((int)_gestures.First(), (int)_gestures.Last(), "Please choose your Gesture: ");
 }
 public static string GetReason(Gesture winner, Gesture loser)
 {
 Tuple<int, int> k = _actions.Keys.Where(key => key.Item1 == (int)winner && key.Item2 == (int)loser).FirstOrDefault();
 foreach (var key in _actions.Keys)
 {
 int w = (int)winner;
 int l = (int)loser;
 if (key.Item1 == w && key.Item2 == l)
 k = key;
 }
 return winner + " " + _actions[k] + " " + loser;
 }
}

Edit: I recently realized that using math to determine the winner is all fun and games, but really we created a Dictionary of who beats who called _actions, why not use it? So I've re-written the WhoWon Method, which is now slightly longer, but more easily scaled, and not dependent on the Gesture enums being in a certain order to work. This replaces # 12

public static int WhoWon(Gesture player1, Gesture player2)
{
 return player1 == player2 ? 0 :_actions.Keys.Where(key => key.Item1 == (int)player1 && key.Item2 == (int)player2).FirstOrDefault() != null ? 1 : 2;
}

• Vogel612: Concerning point 5. There is a big theoretic point in doing this: readability / selfdocumentation. It's just not done right. Same applies for point 7, where OP has done it right. Try to respect OP in your reviews and explain why you changed stuff. Also if you recommend not using something, why did you include it?
• BenVlodgi: @Vogel612 Perhaps it is just an opinion, but I don't believe there is any reason to move any less than 4 lines of code to a method unless it is or could be called more than once. I included code that I didn't recommend, because I wanted him to be aware of the 2 techniques which produced the same result, and explain why chose to go with what seemed like more complex code.
• Marc-Andre: @BenVlodgi I will strongly disagree with your point point, extracting 4 lines of code in a function even if call once is a good thing! This can demonstrate that those 4 lines of codes are doing something special that is a subset of what the big function is doing. This helps when you read the code.
• BenVlodgi: @Marc-Andre In his case, I would have preferred he had just wrapped that code with #region Tags. I do agree that separation of code based on function is good, but I felt that the two places it was done were not the 2 places it NEEDed to be done. To expand more on that, I wouldn't have had such a problem with his extracting 2 lines of code for a method if he had also extracted the 57 lines which determined the winner to a method. To me, that was a much more important change to make in the name of readability.
• Malachi: I would rather not use a do while

Answer 3

In my original review of this program I mentioned I really wanted to re-do this in OO. So I have. I recommend you read my first review to get most of the reasons behind much of the code refactoring I did.

1. Right off the bat I created two enums which will be helpful in the rest of the program. a Gesture enum which holds all possible playable Gestures, and a Performance enum to communicate to a player how they did during a Game.

   enum Gesture
   {
    Rock = 1,
    Paper = 2,
    Scissors = 3,
    Spock = 4,
    Lizard = 5
   }
   enum Performance
   {
    Lost = -1,
    Tied = 0,
    Won = 1
   }
   

2. I created an abstract Player class which handled the specific player data. From here I created a Human and Computer class, which inherits from Player, and implemented their GetMove methods, because that is the only place where these two types of players differ.

   abstract class Player
   {
    public uint Wins { get; private set; }
    public uint Loses { get; private set; }
    public uint Ties { get; private set; }
    public abstract Gesture GetMove();
    public string GetScoreCard()
    {
    return "[Wins: " + Wins + "] [Loses " + Loses + "] [Ties " + Ties + "]";
    }
    public void ClearScore()
    {
    Wins = Loses = Ties = 0;
    }
    public void GiveResult(Performance performance)
    {
    switch (performance)
    {
    case Performance.Lost: Loses++; break;
    case Performance.Tied: Ties++; break;
    case Performance.Won: Wins++; break;
    }
    }
   }
   class Human : Player
   {
    public override Gesture GetMove()
    {
    Utils.PrintMenu(Game.Gestures.Select(g => g.ToString()).ToList(), 1);
    return (Gesture)Utils.PromptForRangedInt((int)Game.Gestures.First(), (int)Game.Gestures.Last(), "Please choose your Gesture: ");
    }
   }
   class Computer : Player
   {
    public override Gesture GetMove()
    {
    return (Gesture)Game.Gestures.GetValue(new Random().Next(Game.Gestures.Length));
    }
   }
   

3. I created a static Game class. This one is static because I didn't think it was necessary to create a new Game object every-time you wanted to play. Instead I implemented a Play method which simply handled all the necessary logic. In the Game class, I enclosed things like, a List of all the Gestures, a Dictionary of Rules (what defeats what and why). A Play method to simulate the RPSLS, a WhoWon method which returns who the winner between two players was, and a GetReason method which returns the reason a particular Gesture won over another.

   static class Game
   {
    public static Gesture[] Gestures = (Gesture[])Enum.GetValues(typeof(Gesture));
    private static Dictionary<Tuple<int, int>, string> Rules = new Dictionary<Tuple<int, int>, string>()
    {
    {Tuple.Create<int,int>(1,3), "Crushes"},
    {Tuple.Create<int,int>(1,5), "Crushes"},
    {Tuple.Create<int,int>(2,1), "Covers"},
    {Tuple.Create<int,int>(2,4), "Disproves"},
    {Tuple.Create<int,int>(3,2), "Cuts"},
    {Tuple.Create<int,int>(3,5), "Decapitates"},
    {Tuple.Create<int,int>(4,3), "Smashes"},
    {Tuple.Create<int,int>(4,1), "Vaporizes"},
    {Tuple.Create<int,int>(5,2), "Eats"},
    {Tuple.Create<int,int>(5,4), "Poisons"}
    };
    public static void Play(Player player1, Player player2)
    {
    Gesture p1move = player1.GetMove();
    Gesture p2move = player2.GetMove();
    Console.Write("Player 1 Chose ");
    Utils.WriteLineColored(p1move.ToString(), ConsoleColor.Green);
    Console.Write("Player 2 Chose ");
    Utils.WriteLineColored(p2move.ToString(), ConsoleColor.Green);
    int result = WhoWon(p1move, p2move);
    switch (result)
    {
    case 0: player1.GiveResult(Performance.Tied); player2.GiveResult(Performance.Tied); break;
    case 1: player1.GiveResult(Performance.Won); player2.GiveResult(Performance.Lost); break;
    case 2: player1.GiveResult(Performance.Lost); player2.GiveResult(Performance.Won); break;
    }
    if (result == 0)
    Console.WriteLine("It was a tie!");
    else
    Console.WriteLine("Player {0} won, because {1}.", result, GetReason(result == 1 ? p1move : p2move, result == 1 ? p2move : p1move));
    }
    private static int WhoWon(Gesture p1move, Gesture p2move)
    {
    return p1move == p2move ? 0 : Rules.Keys.Where(key => key.Item1 == (int)p1move && key.Item2 == (int)p2move).FirstOrDefault() != null ? 1 : 2;
    }
    private static string GetReason(Gesture winner, Gesture loser)
    {
    return winner + " " + Rules[Tuple.Create((int)winner, (int)loser)] + " " + loser;
    }
   }
   

4. For any general utility methods, I enclosed those in a static Utils class

   static class Utils
   {
    public static int PromptForRangedInt(int min = int.MinValue, int max = int.MaxValue, string prompt = "Please enter an Integer: ")
    {
    int g;
    do
    {
    Console.Write(prompt);
    if (int.TryParse(Console.ReadLine(), out g))
    {
    if (g >= min && g <= max)
    return g;
    Console.WriteLine("You entered {0}, but the input must be in the range of ({1} - {2}. Please try again...", g, min, max);
    }
    else
    Console.WriteLine("That is not a number. Please try again...");
    } while (true);
    }
    public static void PrintMenu(List<string> values, int baseIndex = 0)
    {
    values.ForEach(value => Console.WriteLine("{0}: {1}", baseIndex++, value));
    }
    public static void WriteLineColored(string text, ConsoleColor color)
    {
    var curr = Console.ForegroundColor;
    Console.ForegroundColor = color;
    Console.WriteLine(text);
    Console.ForegroundColor = curr;
    }
   }
   

5. This brings me to actually using these classes in a meaningful way to actually play the game. You must create the players you want to pit against each other, this could be 2 computers, 2 humans, or a combination of either. As-long as you hold a reference to these objects, you also are holding onto their scorecard (assuming they didn't clear it).

   class Program
   {
    static void Main(string[] args)
    {
    var gameMenu = new string[] { "Play", "Clear Score", "Quit" };
    var me = new Human();
    var computer = new Computer();
    var playAgain = true;
    do
    {
    Utils.WriteLineColored("Options:", ConsoleColor.White);
    Utils.PrintMenu(gameMenu.ToList());
    switch(Utils.PromptForRangedInt(0, gameMenu.Length - 1, "Choose an Option: "))
    {
    case 0:
    Console.Clear();
    Game.Play(me, computer);
    Console.WriteLine("Your scorecard: " + me.GetScoreCard() + Environment.NewLine);
    break;
    case 1:
    Console.Clear();
    me.ClearScore();
    Utils.WriteLineColored("Your score has been cleared", ConsoleColor.Green);
    break;
    case 2:
    Console.Clear();
    playAgain = false;
    Console.Write("Good bye, thanks for playing!\nPress any Key to contine...");
    Console.ReadKey(true);
    break;
    }
    } while (playAgain);
    }
   }
   

---

Here is the entire dump of the program:

using System;
using System.Collections.Generic;
using System.Linq;
namespace RPSLS
{
 class Program
 {
 static void Main(string[] args)
 {
 var gameMenu = new string[] { "Play", "Clear Score", "Quit" };
 var me = new Human();
 var computer = new Computer();
 var playAgain = true;
 do
 {
 Utils.WriteLineColored("Options:", ConsoleColor.White);
 Utils.PrintMenu(gameMenu.ToList());
 switch(Utils.PromptForRangedInt(0, gameMenu.Length - 1, "Choose an Option: "))
 {
 case 0:
 Console.Clear();
 Game.Play(me, computer);
 Console.WriteLine("Your scorecard: " + me.GetScoreCard() + Environment.NewLine);
 break;
 case 1:
 Console.Clear();
 me.ClearScore();
 Utils.WriteLineColored("Your score has been cleared", ConsoleColor.Green);
 break;
 case 2:
 Console.Clear();
 playAgain = false;
 Console.Write("Good bye, thanks for playing!\nPress any Key to contine...");
 Console.ReadKey(true);
 break;
 }
 } while (playAgain);
 }
 }
 enum Gesture
 {
 Rock = 1,
 Paper = 2,
 Scissors = 3,
 Spock = 4,
 Lizard = 5
 }
 enum Performance
 {
 Lost = -1,
 Tied = 0,
 Won = 1
 }
 abstract class Player
 {
 public uint Wins { get; private set; }
 public uint Loses { get; private set; }
 public uint Ties { get; private set; }
 public abstract Gesture GetMove();
 public string GetScoreCard()
 {
 return "[Wins: " + Wins + "] [Loses " + Loses + "] [Ties " + Ties + "]";
 }
 public void ClearScore()
 {
 Wins = Loses = Ties = 0;
 }
 public void GiveResult(Performance performance)
 {
 switch (performance)
 {
 case Performance.Lost: Loses++; break;
 case Performance.Tied: Ties++; break;
 case Performance.Won: Wins++; break;
 }
 }
 }
 class Human : Player
 {
 public override Gesture GetMove()
 {
 Utils.PrintMenu(Game.Gestures.Select(g => g.ToString()).ToList(), 1);
 return (Gesture)Utils.PromptForRangedInt((int)Game.Gestures.First(), (int)Game.Gestures.Last(), "Please choose your Gesture: ");
 }
 }
 class Computer : Player
 {
 public override Gesture GetMove()
 {
 return (Gesture)Game.Gestures.GetValue(new Random().Next(Game.Gestures.Length));
 }
 }
 static class Game
 {
 public static Gesture[] Gestures = (Gesture[])Enum.GetValues(typeof(Gesture));
 private static Dictionary<Tuple<int, int>, string> Rules = new Dictionary<Tuple<int, int>, string>()
 {
 {Tuple.Create<int,int>(1,3), "Crushes"},
 {Tuple.Create<int,int>(1,5), "Crushes"},
 {Tuple.Create<int,int>(2,1), "Covers"},
 {Tuple.Create<int,int>(2,4), "Disproves"},
 {Tuple.Create<int,int>(3,2), "Cuts"},
 {Tuple.Create<int,int>(3,5), "Decapitates"},
 {Tuple.Create<int,int>(4,3), "Smashes"},
 {Tuple.Create<int,int>(4,1), "Vaporizes"},
 {Tuple.Create<int,int>(5,2), "Eats"},
 {Tuple.Create<int,int>(5,4), "Poisons"}
 };
 public static void Play(Player player1, Player player2)
 {
 Gesture p1move = player1.GetMove();
 Gesture p2move = player2.GetMove();
 Console.Write("Player 1 Chose ");
 Utils.WriteLineColored(p1move.ToString(), ConsoleColor.Green);
 Console.Write("Player 2 Chose ");
 Utils.WriteLineColored(p2move.ToString(), ConsoleColor.Green);
 int result = WhoWon(p1move, p2move);
 switch (result)
 {
 case 0: player1.GiveResult(Performance.Tied); player2.GiveResult(Performance.Tied); break;
 case 1: player1.GiveResult(Performance.Won); player2.GiveResult(Performance.Lost); break;
 case 2: player1.GiveResult(Performance.Lost); player2.GiveResult(Performance.Won); break;
 }
 if (result == 0)
 Console.WriteLine("It was a tie!");
 else
 Console.WriteLine("Player {0} won, because {1}.", result, GetReason(result == 1 ? p1move : p2move, result == 1 ? p2move : p1move));
 }
 private static int WhoWon(Gesture p1move, Gesture p2move)
 {
 return p1move == p2move ? 0 : Rules.Keys.Where(key => key.Item1 == (int)p1move && key.Item2 == (int)p2move).FirstOrDefault() != null ? 1 : 2;
 }
 private static string GetReason(Gesture winner, Gesture loser)
 {
 return winner + " " + Rules[Tuple.Create((int)winner, (int)loser)] + " " + loser;
 }
 }
 static class Utils
 {
 public static int PromptForRangedInt(int min = int.MinValue, int max = int.MaxValue, string prompt = "Please enter an Integer: ")
 {
 int g;
 do
 {
 Console.Write(prompt);
 if (int.TryParse(Console.ReadLine(), out g))
 {
 if (g >= min && g <= max)
 return g;
 Console.WriteLine("You entered {0}, but the input must be in the range of ({1} - {2}. Please try again...", g, min, max);
 }
 else
 Console.WriteLine("That is not a number. Please try again...");
 } while (true);
 }
 public static void PrintMenu(List<string> values, int baseIndex = 0)
 {
 values.ForEach(value => Console.WriteLine("{0}: {1}", baseIndex++, value));
 }
 public static void WriteLineColored(string text, ConsoleColor color)
 {
 var curr = Console.ForegroundColor;
 Console.ForegroundColor = color;
 Console.WriteLine(text);
 Console.ForegroundColor = curr;
 }
 }
}