12
\$\begingroup\$

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;
        }
    }
}    
\$\endgroup\$
  • 1
    \$\begingroup\$ have you seen the update from BenVlodgi on your old question? codereview.stackexchange.com/a/43654/37660 \$\endgroup\$ – Vogel612 Mar 11 '14 at 8:29
  • \$\begingroup\$ are you talking about figuring out the DecideWinner method? I just haven't gotten to that method yet. I like the tuple idea, but there was something that someone said on someone else's RPSLS post that really stuck with me that I want to try. \$\endgroup\$ – Malachi Mar 11 '14 at 12:43
  • 1
    \$\begingroup\$ @Malachi: As this is presumably a learning exercise, you could also try playing with something like stateless. \$\endgroup\$ – Brian Mar 11 '14 at 17:09
  • \$\begingroup\$ @Brian, that might take me a little time to figure out, but looks like a lot of fun as well. are you talking about using this for the DecideWinner method? \$\endgroup\$ – Malachi Mar 11 '14 at 17:19
  • \$\begingroup\$ Yes. I admit that using stateless for DecideWinner is massive overkill, but from a learning perspective it makes for a fun approach. \$\endgroup\$ – Brian Mar 11 '14 at 18:07
10
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I actually tried Number 5 and the Compiler didn't like it. or maybe I tried to make it Global.... \$\endgroup\$ – Malachi Mar 11 '14 at 17:53
  • \$\begingroup\$ @Malachi making it class level and/or the way I showed both work. \$\endgroup\$ – BenVlodgi Mar 11 '14 at 17:55
  • \$\begingroup\$ I was actually thinking that I should move the reset question before the new game question. and possibly give the user a "don't show this again" option. \$\endgroup\$ – Malachi Mar 11 '14 at 17:59
  • 1
    \$\begingroup\$ @Malachi try running your program with just that updated... you'll find it is nicer, and to be honest it would be more annoying if you asked them every time to never show this again, thats just an extra prompt to say no to. Although to be honest they will probably say yes just to avoid 2 popups, which will mean they will then forever lose that option. \$\endgroup\$ – BenVlodgi Mar 11 '14 at 18:01
  • 2
    \$\begingroup\$ just a tip. you can create keyboard keys by using the pseudotag <kbd> \$\endgroup\$ – Vogel612 Mar 12 '14 at 7:09
10
\$\begingroup\$

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.

\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ This formula is highly non-obvious (at least to me). Encoding the who-wins-who into a data structure would be very much preferable, but even the original code has the advantage over your shortcut that it's easy to understand and extend. This is Code Review, not Code Golf ;-) \$\endgroup\$ – amon Mar 11 '14 at 16:56
  • 3
    \$\begingroup\$ @amon: I agree that it is non-obvious. RPS-like games have the following structure: we agree on an odd number, let's say 5. Now we both pick a number from 0 through 4. We take their difference mod 5, producing a game number that is also between 0 and 4. If the game number is zero then we tie. Otherwise, we have 1, 2, 3 or 4 to divide between us. I take the evens, you take the odds, and there's therefore a 50-50 chance for each of us, so its a fair game. Now does the formula make sense? \$\endgroup\$ – Eric Lippert Mar 11 '14 at 17:04
  • 1
    \$\begingroup\$ @amon: Now all we have to do is assign funny names to each choice and order them such that the formula produces an "x beats y" relationship that makes sense with the funny names. But that's just window dressing; the game fundamentally is "pick two numbers, take their difference, assign half the possible differences to each side" whether it is rock-paper-scissors, cowboy-ninja-bear or rock-paper-scissors-spock-lizard-dog-gun. \$\endgroup\$ – Eric Lippert Mar 11 '14 at 17:08
  • 4
    \$\begingroup\$ Although it is a functional formula, I wouldn't use it. Not only because it can be considered obfuscation, but mostly because it would break the logic if you wanted to switch the order of the items. Trust me, it has happened before. \$\endgroup\$ – Simon Forsberg Mar 11 '14 at 17:36
  • 4
    \$\begingroup\$ @SimonAndréForsberg: That's an excellent point; you'd need to make sure that the number associated with an action for the purposes of the calculation and the number associated with the order in which they are displayed are carefully made distinct. \$\endgroup\$ – Eric Lippert Mar 11 '14 at 18:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.