**Edit**: Added 8 - 14
1. An enum will serve you much better than a list of strings.
enum Gesture
{
Rock = 1,
Paper = 2,
Scissors = 3,
Lizard = 4,
Spock = 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
{
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 > player2 and the sum 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 `OR`s
public static int WhoWon(Gesture player1, Gesture player2)
{
return player1 == player2 ? 0 : ((player1 > player2) == (((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<Tuple<int,int>, string> with all the possible actions which will happen when a specific Gesture wins over another. I used Tuple<int,int> 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'); return true;
case ConsoleKey.N: Console.Write('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);
}
----------
Note: I ran this code now for the first time, and found there is a problem in using the Tuple<int,int> as a key in the Dictionary.. I'm not sure why yet, still working on that