In the scope of this program, there is no benefit to using a
Gestureclassvs using anenumwith some extension methods. I will draw upon my latest version of an OOP RPSLSOOP RPSLS for some examples.Using a
Gestureenumenum Gesture { Rock, Paper, Scissors, Lizard, Spock }You can easily add an extension method to give you the
Defeatsmethod you implemented in yourGestureclass. Below I assume you have a Dictionary of Rules like in my versionmy version.public static class Extensions { public static bool Defeates(this Gesture me, Gesture gesture) { return Game.Rules[me].Item1.ContainsKey(gesture); } }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 to a List ... 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
Twill be inferred from the type ofListyou pass in and will result with that type ofobjectbeing returned. I also in-lined the declaration of yourRandomobjectwith the call of itsNextmethod.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)); }Your win/lose/tie variables should be plural.
int wins = 0; int loses = 0; int ties = 0;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; }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 CtrlED 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++;You should use OO for players instead of for Gestures, you can see my old reviewold review for more info on that. Or look at my more recent CR postCR post.
Anything else I would have pointed out, has already been said by @Mat's Mug@Mat's Mug
In the scope of this program, there is no benefit to using a
Gestureclassvs using anenumwith some extension methods. I will draw upon my latest version of an OOP RPSLS for some examples.Using a
Gestureenumenum Gesture { Rock, Paper, Scissors, Lizard, Spock }You can easily add an extension method to give you the
Defeatsmethod you implemented in yourGestureclass. Below I assume you have a Dictionary of Rules like in my version.public static class Extensions { public static bool Defeates(this Gesture me, Gesture gesture) { return Game.Rules[me].Item1.ContainsKey(gesture); } }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 to a List ... 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
Twill be inferred from the type ofListyou pass in and will result with that type ofobjectbeing returned. I also in-lined the declaration of yourRandomobjectwith the call of itsNextmethod.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)); }Your win/lose/tie variables should be plural.
int wins = 0; int loses = 0; int ties = 0;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; }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 CtrlED 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++;You should use OO for players instead of for Gestures, you can see my old review for more info on that. Or look at my more recent CR post.
Anything else I would have pointed out, has already been said by @Mat's Mug
In the scope of this program, there is no benefit to using a
Gestureclassvs using anenumwith some extension methods. I will draw upon my latest version of an OOP RPSLS for some examples.Using a
Gestureenumenum Gesture { Rock, Paper, Scissors, Lizard, Spock }You can easily add an extension method to give you the
Defeatsmethod you implemented in yourGestureclass. Below I assume you have a Dictionary of Rules like in my version.public static class Extensions { public static bool Defeates(this Gesture me, Gesture gesture) { return Game.Rules[me].Item1.ContainsKey(gesture); } }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 to a List ... 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
Twill be inferred from the type ofListyou pass in and will result with that type ofobjectbeing returned. I also in-lined the declaration of yourRandomobjectwith the call of itsNextmethod.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)); }Your win/lose/tie variables should be plural.
int wins = 0; int loses = 0; int ties = 0;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; }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 CtrlED 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++;You should use OO for players instead of for Gestures, you can see my old review for more info on that. Or look at my more recent CR post.
Anything else I would have pointed out, has already been said by @Mat's Mug
In the scope of this program, there is no benefit to using a
Gestureclassvs using anenumwith some extension methods. I will draw upon my latest version of an OOP RPSLS for some examples.Using a
Gestureenumenum Gesture { Rock, Paper, Scissors, Lizard, Spock }You can easily add an extension method to give you the
Defeatsmethod you implemented in yourGestureclass. Below I assume you have a Dictionary of Rules like in my version.public static class Extensions { public static bool Defeates(this Gesture me, Gesture gesture) { return Game.Rules[me].Item1.ContainsKey(gesture); } }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 to a List ... 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
Twill be inferred from the type ofListyou pass in and will result with that type ofobjectbeing returned. I also in-lined the declaration of yourRandomobjectwith the call of itsNextmethod.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)); }Your win/lose/tie variables should be plural.
int wins = 0; int loses = 0; int ties = 0;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; }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 CtrlED 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++;You should use OO for players instead of for Gestures, you can see my old review for more info on that. Or look at my more recent CR post.
Anything else I would have pointed out, has already been said by @Mat's Mug