RPSLS is less messy now, but is it clean?

Question

This is a follow up to the following questions:

RPSLS Game in C#

Ensuring user input is an integer in a range

I haven't made my way to DecideWinner() yet, but that is next on my To-Do List. I would like the majority of the focus to go into what I have done with the rest of the code, but would still enjoy some input on what to do with my nasty if then else statements.

I did not change to using an Enumeration for my Gestures. I read the Microsoft page for Enumeration and it didn't seem like it would be a good fit, maybe I didn't understand fully though either.

I also used while loops instead of do while loops because I like them better. not really a good reason I guess other than that I prefer while loops.

I am not afraid to say that I am still learning, so here is the code:

/// <summary>
/// Thanks to 
/// https://codereview.stackexchange.com/users/38054/benvlodgi
/// https://codereview.stackexchange.com/users/4318/eric-lippert
/// https://codereview.stackexchange.com/users/23788/mats-mug
/// https://codereview.stackexchange.com/users/30346/chriswue
/// </summary>
namespace RPSLSGame
{
    class MainClass
    {
        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> Gestures = new List<string>{"rock","paper","scissors", "lizard","spock"};

            int win = 0;
            int lose = 0;
            int tie = 0;

            var newGame = true;
            while (newGame)
            {
                var playerGesture = "";
                Console.WriteLine("Please choose your Gesture ");
                PrintMenu(Gestures);
                playerGesture = Gestures[PromptForNumber("Please choose your gesture",1,Gestures.Count)-1];
                var computerPlay = GetRandomOption(Gestures);

                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);
                if(!Choice ("Would you like to play again? Y/N"))
                {
                    if (Choice("Would you like to reset your score?"))
                    {
                        win = 0;
                        lose=0;
                        tie=0;
                    }
                    if (!Choice ("Would you like to play another game?"))
                    {
                        newGame=false;
                    }
                }
            }
            Console.WriteLine("Goodbye");
            Console.ReadLine ();
        }

        public static void DecideWinner ()
        {
            //TODO: Create Method for Deciding the Winner.
        }

        static int PromptForNumber (string prompt, int lower, int upper)
        {
            int? pick = null;
            while (pick == null) {
                Console.WriteLine(prompt);
                pick = Console.ReadLine().BoundedParse (lower, upper);
            }
            return pick.Value;
        }

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

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

        public static Boolean Choice (string prompt)
        {
            while(true)
            {
                Console.WriteLine (prompt);
                switch (Console.ReadKey (true).Key)
                {
                    case ConsoleKey.Y:
                    {
                        Console.Write ("Y\n"); 
                        return true;
                    }
                    case ConsoleKey.N:
                    {
                        Console.Write ("N\n");
                        return false;
                    }
                }
            }
        }
    }

    static class Extensions
    {

            /// <summary>
        /// Parses the Input for an Integer between the lower and upper bounds
        /// </summary>
        /// <returns>
        /// null or integer
        /// </returns>
        /// <param name='str'>
        /// String to be parsed for an Integer
        /// </param>
        /// <param name='lower'>
        /// Lower Bound of the Acceptable range of input integers
        /// </param>
        /// <param name='upper'>
        /// Upper bound of the acceptable range of the input integers
        /// </param>
        public static int? BoundedParse (this string str, int lower, int upper)
        {
            if (str == null) {
                return null;
            }
            int result;
            bool success;
            success = int.TryParse (str, out result);
            if (!success) {
                return null;
            }
            if (result < lower) {
                return null;
            }
            if (result > upper) {
                return null;
            }
            return result;
        }
    }
}    

Answer 1

Some of what I say may be repeats of what I said in my original review.

1. Rename your win, lose, tie counters to _wins, _loses, _ties. They should be plural, underscore is optional. It is easy to mass rename variables in VS, just click on the variable in the text editor and press Ctrl + R + R and a dialog will popup which will allow you to rename that symbol through your whole project.

2. Remove that massive if-else block in the middle of your program, it looks as though you were going to separate that out and use the DecideWinner method you created. Here again is how I did it before with my original review.

   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;
   }
   

3. From a usability standpoint, it is annoying to a user when they have to answer the same question twice. In this case, after playing a game, they are asked twice if they want to quit. I've re-written the end of your loop to better handle this situation.

   Console.WriteLine("Your Score is (W:L:T:) : {0}:{1}:{2}", win, lose, tie);
   
   newGame = Choice("Would you like to play another game?");
   
   if (newGame && Choice("Would you like to reset your score?"))
       win = lose = tie = 0;
   

   Now the user is only asked once if they want to play again, and if they do choose to play again, only then are they asked if they want to reset their score.

4. Instead of using a nullable int, I decided to follow the pattern used by Microsoft with their TryParse

   static class Extensions
   {
       /// <summary>Tries to parse the string, returning true if successful and if the parsed value falls between the given bounds.</summary>
       /// <param name="str">The string to parse.</param>
       /// <param name="lower">The lower bound.</param>
       /// <param name="upper">The upper bound.</param>
       /// <param name="value">The parsed value.</param>
       public static bool TryBoundedParse(this string str, int lower, int upper, out int value)
       {
           return int.TryParse(str, out value) && value >= lower && value <= upper;
       }
   }
   

   This would result in your PromptForNumber looking like this.

   public static int PromptForNumber (int min = int.MinValue, int max = int.MaxValue, string prompt = "Please enter an Integer: ")
   {
       int parsedValue;
       do { Console.Write(prompt); }
       while (!Console.ReadLine().TryBoundedParse(min, max, out parsedValue));
       return parsedValue;
   }
   

   If you do still want to use the BoundedParse Method you could simplify it by writing it like this instead. Which simply sums up all of your conditions into one statement, and doesn't utilize early returns as I know you like to avoid.

   public static int? BoundedParse(this string str, int lower, int upper)
   {
       int result;
       return (int.TryParse(str, out result) && result >= lower && result <= upper) ? (int?)result : null;
   }
   

5. In your GetRandomOption you can inline the creation of the Random object with your call to get a random option, which would save you a whole line of code :P

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

6. The Choice method does not need to use the { brackets } because switch statements end when there is a break or a return. So you can write it like this

   Console.WriteLine(prompt);
   switch (Console.ReadKey(true).Key)
   {
       case ConsoleKey.Y:
           Console.Write("Y\n");
           return true;
       case ConsoleKey.N:
           Console.Write("N\n");
           return false;
   }
   

   However it looks way cooler if you just one line the case statements because they have identical layouts and similar functionality. Which makes it easy to distinguish between the statements at a glance and use a DO-WHILE!... because they're awesome.

   public static Boolean Choice(string prompt)
   {
       do
       {
           Console.WriteLine(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);
   }
   

7. Use enums from the start and not a list of strings. Then just look at my entire suggested implementation using enums.

Answer 2

I like the while loop. It states the exit condition from the start, so you know right away what you're getting yourself into.

However I don't like the multi-screen if block.

What if you had a method to handle each of the player's possible moves? Say you have a method to increment losses and another to increment wins - you'd have to promote win, lose and tie to instance variables for this to work:

private void PlayerWins(string playerGesture, string verb, string computerGesture)
{
    win++;
    Console.WriteLine ("You Win, {0} {1} {2}", playerGesture, verb, computerGesture);
}

private void ComputerWins(string computerGesture, string verb, string playerGesture)
{
    lose++;
    Console.WriteLine ("You Lose, {0} {1} {2}", computerGesture, verb, playerGesture);
}

private void NobodyWins()
{
    tie++;
    Console.WriteLine ("Tie!");
}

Then you could have a method to handle each player moves:

private void PlayRock(string computerGesture)
{
    var playerGesture = "rock";
    if (computerGesture == "lizard" || computerGesture == "scissors")
    {
        PlayerWins(playerGesture, "crushes", computerGesture);
    }
    else if (computerGesture == "paper")
    {
        ComputerWins(computerGesture, "covers", playerGesture);
    }
    else if (computerGesture == "spock")
    {
        ComputerWins(computerGesture, "vaporizes", playerGesture);
    }
}

And then your main loop could "branch" on a method depending on the player's move - you can set up a Dictionary<string,Action<string>> to do that (Action<string> is a delegate that returns void and takes a string parameter.. which matches the signatures of your PlayXXXXX methods):

var plays = new Dictionary<string,Action<string>> {
        { "rock", PlayRock },
        { "paper", PlayPaper },
        { "scissors", PlayScissors },
        { "lizard", PlayLizard },
        { "spock", PlaySpock }
    };

This way you can invoke the appropriate Action delegate simply by getting the dictionary entry for the playerGesture:

plays[playerGesture](computerGesture);

...this invocation replaces the entire if block in the main loop, and as a bonus you now have a separate method for each playable move.

Answer 3

I agree with Mat's Mug that the huge if-block in the middle of the loop makes the loop hard to read. I'd factor that out into its own method.

I'd also think about ways to minimize that huge if-block into something smaller.

Let's consider the nature of rock-paper-scissors-spock-lizard. If we assign numbers

rock = 0
paper = 1 
scissors = 2
spock = 3
lizard = 4

then (assuming you've already taken out ties) x beats y can be computed as

bool xWins = (((y + 5 - x) % 5) % 2) == 0;

Try it: if x is scissors (2) and y is lizard (4) then y + 5 - x is 7, that gives remainder 2 when divided by 5, and that gives remainder 0 when divided by 2, so the expression is true, and in fact scissors does beat lizard.

You can use this formula to greatly simplify the computation of whether the player wins, ties or loses. Once you know that, you can use a table lookup to figure out what "x beats y" message to display.