Skip to main content
Code Review

Return to Question

replaced http://codereview.stackexchange.com/ with https://codereview.stackexchange.com/
Source Link

I wrote a couple reviews for this CR post CR post. In my most recent review, I refactored @Malachi 's code to fit OO design. I'm looking for any advice/hints/criticisms on it.

I wrote a couple reviews for this CR post. In my most recent review, I refactored @Malachi 's code to fit OO design. I'm looking for any advice/hints/criticisms on it.

I wrote a couple reviews for this CR post. In my most recent review, I refactored @Malachi 's code to fit OO design. I'm looking for any advice/hints/criticisms on it.

deleted 1515 characters in body
Source Link
Jamal
  • 35.2k
  • 13
  • 134
  • 238

A Reviewreview is welcome for both the OO design I implemented and the Main`main() method which... is a bit sloppy, but feel free to review both!.

using System;
using System.Collections.Generic;
using System.Linq;
namespace RPSLS
{
 class Program
 {
 static void Main(string[] args)
 {
 var endGameMenu = new string[] { "Play Again", "Clear Score", "Quit" };
 var me = new Human();
 var computer = new Computer();
 var playAgain = true;
 do
 {
 Game.Play(me, computer);
 Console.WriteLine("Your scorecard: " + me.GetScoreCard());
 int result;
 do
 {
 Console.WriteLine("Options:");
 Utils.PrintMenu(endGameMenu.ToList());
 result = Utils.PromptForRangedInt(0, endGameMenu.Length - 1, "Choose an Option: ");
 if (result == 1)
 {
 me.ClearScore();
 Console.Clear();
 Utils.WriteLineColored("Your score has been cleared", ConsoleColor.Green);
 }
 } while (result != 0 && result != 2);
 Console.Clear();
 playAgain = result == 0;
 } while (playAgain);
 }
 }
 enum Gesture
 {
 Rock = 1,
 Paper = 2,
 Scissors = 3,
 Spock = 4,
 Lizard = 5
 }
 enum Performance
 {
 Lost = -1,
 Tied = 0,
 Won = 1
 }
 abstract class Player
 {
 public uint Wins { get; private set; }
 public uint Loses { get; private set; }
 public uint Ties { get; private set; }
 public abstract Gesture GetMove();
 public string GetScoreCard()
 {
 return "[Wins: " + Wins + "] [Loses " + Loses + "] [Ties " + Ties + "]";
 }
 public void ClearScore()
 {
 Wins = Loses = Ties = 0;
 }
 public void GiveResult(Performance performance)
 {
 switch (performance)
 {
 case Performance.Lost: Loses++; break;
 case Performance.Tied: Ties++; break;
 case Performance.Won: Wins++; break;
 }
 }
 }
 class Human : Player
 {
 public override Gesture GetMove()
 {
 Utils.PrintMenu(Game.Gestures.Select(g => g.ToString()).ToList(), 1);
 return (Gesture)Utils.PromptForRangedInt((int)Game.Gestures.First(), (int)Game.Gestures.Last(), "Please choose your Gesture: ");
 }
 }
 class Computer : Player
 {
 public override Gesture GetMove()
 {
 return (Gesture)Game.Gestures.GetValue(new Random().Next(Game.Gestures.Length));
 }
 }
 static class Game
 {
 public static Gesture[] Gestures = (Gesture[])Enum.GetValues(typeof(Gesture));
 private static Dictionary<Tuple<int, int>, string> Rules = 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"}
 };
 public static void Play(Player player1, Player player2)
 {
 Gesture p1move = player1.GetMove();
 Gesture p2move = player2.GetMove();
 Console.Write("Player 1 Chose ");
 Utils.WriteLineColored(p1move.ToString(), ConsoleColor.Green);
 Console.Write("Player 2 Chose ");
 Utils.WriteLineColored(p2move.ToString(), ConsoleColor.Green);
 int result = WhoWon(p1move, p2move);
 switch (result)
 {
 case 0: player1.GiveResult(Performance.Tied); player2.GiveResult(Performance.Tied); break;
 case 1: player1.GiveResult(Performance.Won); player2.GiveResult(Performance.Lost); break;
 case 2: player1.GiveResult(Performance.Lost); player2.GiveResult(Performance.Won); break;
 }
 if (result == 0)
 Console.WriteLine("It was a tie!");
 else
 Console.WriteLine("Player {0} won, because {1}.", result, GetReason(result == 1 ? p1move : p2move, result == 1 ? p2move : p1move));
 }
 private static int WhoWon(Gesture p1move, Gesture p2move)
 {
 return p1move == p2move ? 0 : Rules.Keys.Where(key => key.Item1 == (int)p1move && key.Item2 == (int)p2move).FirstOrDefault() != null ? 1 : 2;
 }
 private static string GetReason(Gesture winner, Gesture loser)
 {
 return winner + " " + Rules[Tuple.Create((int)winner, (int)loser)] + " " + loser;
 }
 }
 static class Utils
 {
 public static int PromptForRangedInt(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 number. Please try again...");
 } while (true);
 }
 public static void PrintMenu(List<string> values, int baseIndex = 0)
 {
 values.ForEach(value => Console.WriteLine("{0}: {1}", baseIndex++, value));
 }
 public static void WriteLineColored(string text, ConsoleColor color)
 {
 var curr = Console.ForegroundColor;
 Console.ForegroundColor = color;
 Console.WriteLine(text);
 Console.ForegroundColor = curr;
 }
 }
}

Edit: After some early criticism on the Main method in this program I changed it to a better design. For prosperity sake The original dump above is left the same , but here is my updated Main method

static void Main(string[] args)
{
 var gameMenu = new string[] { "Play", "Clear Score", "Quit" };
 var me = new Human();
 var computer = new Computer();
 var playAgain = true;
 do
 {
 Utils.WriteLineColored("Options:", ConsoleColor.White);
 Utils.PrintMenu(gameMenu.ToList());
 switch(Utils.PromptForRangedInt(0, gameMenu.Length - 1, "Choose an Option: "))
 {
 case 0:
 Console.Clear();
 Game.Play(me, computer);
 Console.WriteLine("Your scorecard: " + me.GetScoreCard() + Environment.NewLine);
 break;
 case 1:
 Console.Clear();
 me.ClearScore();
 Utils.WriteLineColored("Your score has been cleared", ConsoleColor.Green);
 break;
 case 2:
 Console.Clear();
 playAgain = false;
 Console.Write("Good bye, thanks for playing!\nPress any Key to contine...");
 Console.ReadKey(true);
 break;
 }
 } while (playAgain);
}

A Review is welcome for both the OO design I implemented and the Main method which... is a bit sloppy, but feel free to review both!

using System;
using System.Collections.Generic;
using System.Linq;
namespace RPSLS
{
 class Program
 {
 static void Main(string[] args)
 {
 var endGameMenu = new string[] { "Play Again", "Clear Score", "Quit" };
 var me = new Human();
 var computer = new Computer();
 var playAgain = true;
 do
 {
 Game.Play(me, computer);
 Console.WriteLine("Your scorecard: " + me.GetScoreCard());
 int result;
 do
 {
 Console.WriteLine("Options:");
 Utils.PrintMenu(endGameMenu.ToList());
 result = Utils.PromptForRangedInt(0, endGameMenu.Length - 1, "Choose an Option: ");
 if (result == 1)
 {
 me.ClearScore();
 Console.Clear();
 Utils.WriteLineColored("Your score has been cleared", ConsoleColor.Green);
 }
 } while (result != 0 && result != 2);
 Console.Clear();
 playAgain = result == 0;
 } while (playAgain);
 }
 }
 enum Gesture
 {
 Rock = 1,
 Paper = 2,
 Scissors = 3,
 Spock = 4,
 Lizard = 5
 }
 enum Performance
 {
 Lost = -1,
 Tied = 0,
 Won = 1
 }
 abstract class Player
 {
 public uint Wins { get; private set; }
 public uint Loses { get; private set; }
 public uint Ties { get; private set; }
 public abstract Gesture GetMove();
 public string GetScoreCard()
 {
 return "[Wins: " + Wins + "] [Loses " + Loses + "] [Ties " + Ties + "]";
 }
 public void ClearScore()
 {
 Wins = Loses = Ties = 0;
 }
 public void GiveResult(Performance performance)
 {
 switch (performance)
 {
 case Performance.Lost: Loses++; break;
 case Performance.Tied: Ties++; break;
 case Performance.Won: Wins++; break;
 }
 }
 }
 class Human : Player
 {
 public override Gesture GetMove()
 {
 Utils.PrintMenu(Game.Gestures.Select(g => g.ToString()).ToList(), 1);
 return (Gesture)Utils.PromptForRangedInt((int)Game.Gestures.First(), (int)Game.Gestures.Last(), "Please choose your Gesture: ");
 }
 }
 class Computer : Player
 {
 public override Gesture GetMove()
 {
 return (Gesture)Game.Gestures.GetValue(new Random().Next(Game.Gestures.Length));
 }
 }
 static class Game
 {
 public static Gesture[] Gestures = (Gesture[])Enum.GetValues(typeof(Gesture));
 private static Dictionary<Tuple<int, int>, string> Rules = 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"}
 };
 public static void Play(Player player1, Player player2)
 {
 Gesture p1move = player1.GetMove();
 Gesture p2move = player2.GetMove();
 Console.Write("Player 1 Chose ");
 Utils.WriteLineColored(p1move.ToString(), ConsoleColor.Green);
 Console.Write("Player 2 Chose ");
 Utils.WriteLineColored(p2move.ToString(), ConsoleColor.Green);
 int result = WhoWon(p1move, p2move);
 switch (result)
 {
 case 0: player1.GiveResult(Performance.Tied); player2.GiveResult(Performance.Tied); break;
 case 1: player1.GiveResult(Performance.Won); player2.GiveResult(Performance.Lost); break;
 case 2: player1.GiveResult(Performance.Lost); player2.GiveResult(Performance.Won); break;
 }
 if (result == 0)
 Console.WriteLine("It was a tie!");
 else
 Console.WriteLine("Player {0} won, because {1}.", result, GetReason(result == 1 ? p1move : p2move, result == 1 ? p2move : p1move));
 }
 private static int WhoWon(Gesture p1move, Gesture p2move)
 {
 return p1move == p2move ? 0 : Rules.Keys.Where(key => key.Item1 == (int)p1move && key.Item2 == (int)p2move).FirstOrDefault() != null ? 1 : 2;
 }
 private static string GetReason(Gesture winner, Gesture loser)
 {
 return winner + " " + Rules[Tuple.Create((int)winner, (int)loser)] + " " + loser;
 }
 }
 static class Utils
 {
 public static int PromptForRangedInt(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 number. Please try again...");
 } while (true);
 }
 public static void PrintMenu(List<string> values, int baseIndex = 0)
 {
 values.ForEach(value => Console.WriteLine("{0}: {1}", baseIndex++, value));
 }
 public static void WriteLineColored(string text, ConsoleColor color)
 {
 var curr = Console.ForegroundColor;
 Console.ForegroundColor = color;
 Console.WriteLine(text);
 Console.ForegroundColor = curr;
 }
 }
}

Edit: After some early criticism on the Main method in this program I changed it to a better design. For prosperity sake The original dump above is left the same , but here is my updated Main method

static void Main(string[] args)
{
 var gameMenu = new string[] { "Play", "Clear Score", "Quit" };
 var me = new Human();
 var computer = new Computer();
 var playAgain = true;
 do
 {
 Utils.WriteLineColored("Options:", ConsoleColor.White);
 Utils.PrintMenu(gameMenu.ToList());
 switch(Utils.PromptForRangedInt(0, gameMenu.Length - 1, "Choose an Option: "))
 {
 case 0:
 Console.Clear();
 Game.Play(me, computer);
 Console.WriteLine("Your scorecard: " + me.GetScoreCard() + Environment.NewLine);
 break;
 case 1:
 Console.Clear();
 me.ClearScore();
 Utils.WriteLineColored("Your score has been cleared", ConsoleColor.Green);
 break;
 case 2:
 Console.Clear();
 playAgain = false;
 Console.Write("Good bye, thanks for playing!\nPress any Key to contine...");
 Console.ReadKey(true);
 break;
 }
 } while (playAgain);
}

A review is welcome for both the OO design I implemented and the `main() method which is a bit sloppy.

using System;
using System.Collections.Generic;
using System.Linq;
namespace RPSLS
{
 class Program
 {
 static void Main(string[] args)
 {
 var endGameMenu = new string[] { "Play Again", "Clear Score", "Quit" };
 var me = new Human();
 var computer = new Computer();
 var playAgain = true;
 do
 {
 Game.Play(me, computer);
 Console.WriteLine("Your scorecard: " + me.GetScoreCard());
 int result;
 do
 {
 Console.WriteLine("Options:");
 Utils.PrintMenu(endGameMenu.ToList());
 result = Utils.PromptForRangedInt(0, endGameMenu.Length - 1, "Choose an Option: ");
 if (result == 1)
 {
 me.ClearScore();
 Console.Clear();
 Utils.WriteLineColored("Your score has been cleared", ConsoleColor.Green);
 }
 } while (result != 0 && result != 2);
 Console.Clear();
 playAgain = result == 0;
 } while (playAgain);
 }
 }
 enum Gesture
 {
 Rock = 1,
 Paper = 2,
 Scissors = 3,
 Spock = 4,
 Lizard = 5
 }
 enum Performance
 {
 Lost = -1,
 Tied = 0,
 Won = 1
 }
 abstract class Player
 {
 public uint Wins { get; private set; }
 public uint Loses { get; private set; }
 public uint Ties { get; private set; }
 public abstract Gesture GetMove();
 public string GetScoreCard()
 {
 return "[Wins: " + Wins + "] [Loses " + Loses + "] [Ties " + Ties + "]";
 }
 public void ClearScore()
 {
 Wins = Loses = Ties = 0;
 }
 public void GiveResult(Performance performance)
 {
 switch (performance)
 {
 case Performance.Lost: Loses++; break;
 case Performance.Tied: Ties++; break;
 case Performance.Won: Wins++; break;
 }
 }
 }
 class Human : Player
 {
 public override Gesture GetMove()
 {
 Utils.PrintMenu(Game.Gestures.Select(g => g.ToString()).ToList(), 1);
 return (Gesture)Utils.PromptForRangedInt((int)Game.Gestures.First(), (int)Game.Gestures.Last(), "Please choose your Gesture: ");
 }
 }
 class Computer : Player
 {
 public override Gesture GetMove()
 {
 return (Gesture)Game.Gestures.GetValue(new Random().Next(Game.Gestures.Length));
 }
 }
 static class Game
 {
 public static Gesture[] Gestures = (Gesture[])Enum.GetValues(typeof(Gesture));
 private static Dictionary<Tuple<int, int>, string> Rules = 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"}
 };
 public static void Play(Player player1, Player player2)
 {
 Gesture p1move = player1.GetMove();
 Gesture p2move = player2.GetMove();
 Console.Write("Player 1 Chose ");
 Utils.WriteLineColored(p1move.ToString(), ConsoleColor.Green);
 Console.Write("Player 2 Chose ");
 Utils.WriteLineColored(p2move.ToString(), ConsoleColor.Green);
 int result = WhoWon(p1move, p2move);
 switch (result)
 {
 case 0: player1.GiveResult(Performance.Tied); player2.GiveResult(Performance.Tied); break;
 case 1: player1.GiveResult(Performance.Won); player2.GiveResult(Performance.Lost); break;
 case 2: player1.GiveResult(Performance.Lost); player2.GiveResult(Performance.Won); break;
 }
 if (result == 0)
 Console.WriteLine("It was a tie!");
 else
 Console.WriteLine("Player {0} won, because {1}.", result, GetReason(result == 1 ? p1move : p2move, result == 1 ? p2move : p1move));
 }
 private static int WhoWon(Gesture p1move, Gesture p2move)
 {
 return p1move == p2move ? 0 : Rules.Keys.Where(key => key.Item1 == (int)p1move && key.Item2 == (int)p2move).FirstOrDefault() != null ? 1 : 2;
 }
 private static string GetReason(Gesture winner, Gesture loser)
 {
 return winner + " " + Rules[Tuple.Create((int)winner, (int)loser)] + " " + loser;
 }
 }
 static class Utils
 {
 public static int PromptForRangedInt(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 number. Please try again...");
 } while (true);
 }
 public static void PrintMenu(List<string> values, int baseIndex = 0)
 {
 values.ForEach(value => Console.WriteLine("{0}: {1}", baseIndex++, value));
 }
 public static void WriteLineColored(string text, ConsoleColor color)
 {
 var curr = Console.ForegroundColor;
 Console.ForegroundColor = color;
 Console.WriteLine(text);
 Console.ForegroundColor = curr;
 }
 }
}
edited tags
Link
Simon Forsberg
  • 59.7k
  • 9
  • 157
  • 311
edited tags
Link
syb0rg
  • 21.9k
  • 10
  • 113
  • 192
Loading
Tweeted twitter.com/#!/StackCodeReview/status/443834697702207489
re-arranged my previous edit
Source Link
BenVlodgi
  • 4.3k
  • 2
  • 21
  • 47
Loading
added 1523 characters in body
Source Link
BenVlodgi
  • 4.3k
  • 2
  • 21
  • 47
Loading
added 136 characters in body
Source Link
BenVlodgi
  • 4.3k
  • 2
  • 21
  • 47
Loading
Source Link
BenVlodgi
  • 4.3k
  • 2
  • 21
  • 47
Loading
lang-cs

AltStyle によって変換されたページ (->オリジナル) /