1. In the scope of this program, there is no benefit to using a `Gesture` `class` vs using an `enum` with some extension methods. I will draw upon my latest version of an [OOP RPSLS][1] for some examples.
Using a `Gesture` `enum`
enum Gesture
{
Rock,
Paper,
Scissors,
Lizard,
Spock
}
You can easily add an extension method to give you the `Defeats` method you implemented in your `Gesture` `class`. Below I assume you have a Dictionary of Rules like in [my version][2].
public static class Extensions
{
public static bool Defeates(this Gesture me, Gesture gesture)
{
return Game.Rules[me].Item1.ContainsKey(gesture);
}
}
2. I see that you have a few duplicated functions which do the exact same thing, only.. with a different data type. This is a bit of a red flag. Instead in this case you should just make one method. The one that takes a list of strings. Then make a function to convert an IList<Gesture> to a List<string> ... this is super simple
myGestureList.select(g=>g.ToString()).ToList();
Or, better yet... make a method that uses Generic T, then you can use PrintMenu GetRandomOption with any data type.
public static T GetRandomOption<T>(List<T> options)
{
return options[new Random().Next(0, options.Count)];
}
Now whenever you call this method the type `T` will be inferred from the type of `List` you pass in and will result with that type of `object` being returned. I also in-lined the declaration of your `Random` `object` with the call of its `Next` method.
Using this same technique, your PrintMenu Function will look like this (I think you should call this `PrintOptions`).
public static void PrintMenu<T>(List<T> List)
{
for (int i = 0; i < List.Count; i++)
{
Console.WriteLine((i + 1) + ": " + List[i]);
}
}
Here is re-worked version of this code, which makes it easy to offset the base number if desired.
public static void PrintMenu<T>(List<T> values, int baseIndex = 0)
{
values.ForEach(value => Console.WriteLine("{0}: {1}", baseIndex++, value));
}
3. Your win/lose/tie variables should be plural.
int wins = 0;
int loses = 0;
int ties = 0;
4. In your Choice Function you should take off the brackets from your case statements.
switch (Console.ReadKey(true).Key)
{
case ConsoleKey.Y:
Console.Write("Y\n");
return true;
case ConsoleKey.N:
Console.Write("N\n");
return false;
}
5. In C# the convention is different than Java where you are encouraged to add your brackets at the end of a statement.. ie.
if(Boolean value){
statement;
}
Your if-else chain should not use that convention. And really you could just not have the brackets at all considering they are 1 liners. I wouldn't even be opposed to 1-lining each of these statements. This point is a little more opinion based, but that is the C# convention. If you just press <kbd>Ctrl</kbd><kbd>E</kbd><kbd>D</kbd> in Visual Studio, you will find that your code will be reformatted to expand those brackets (only works if your code has no errors).
if (playerGesture.Equals(computerPlay))
tie++;
else if(playerGesture.Defeats(computerPlay))
win++;
else
lose++;
6. You should use OO for players instead of for Gestures, you can see my [old review][3] for more info on that. Or look at my more recent [CR post][4].
7. Anything else I would have pointed out, has already been said by [@Mat's Mug][5]
[1]: http://codereview.stackexchange.com/questions/44577/rpslsmb-oop-version-2
[2]: http://codereview.stackexchange.com/questions/44577/rpslsmb-oop-version-2
[3]: http://codereview.stackexchange.com/a/44175/38054
[4]: http://codereview.stackexchange.com/questions/44577/rpslsmb-oop-version-2
[5]: http://codereview.stackexchange.com/users/23788/mats-mug