Skip to main content
replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link
  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 RPSLSOOP RPSLS 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 versionmy version.

     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 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 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 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++; 
  6. 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.

  7. Anything else I would have pointed out, has already been said by @Mat's Mug@Mat's Mug

  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 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.

     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 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 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 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++; 
  6. 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.

  7. Anything else I would have pointed out, has already been said by @Mat's Mug

  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 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.

     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 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 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 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++; 
  6. 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.

  7. Anything else I would have pointed out, has already been said by @Mat's Mug

Source Link
BenVlodgi
  • 4.3k
  • 2
  • 21
  • 47

  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 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.

     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 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 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 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++; 
  6. 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.

  7. Anything else I would have pointed out, has already been said by @Mat's Mug