RPSLS refactored to Object Oriented

Question

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.

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

Here is the entire dump:

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;
 }
 }
}

• Malachi: you should do that the other way around. post the update separate.
• BenVlodgi: @Malachi done ;)

Answer 1

Main has already been taken care of so I'm going to concentrate on the other stuff:

1. enum Performance seems like an odd name considering that it represents the result of a game - calling it Result or GameResult seems more appropriate.

2. Your code makes the implicit assumption that the gestures are numbered consecutively starting at 1 which is not ideal. Your utility functions could just take the enum as a parameter use it to print the menu and parse the proper selection. Something like this

   public static void PrintEnumSelection<T>()
   {
    var t = typeof(T);
    if (!t.IsEnum)
    throw new ArgumentException("type must be an enum");
    Enum.GetValues(typeof(T)).Cast<T>().ToList().ForEach(value => Console.WriteLine("{0}: {1}", (int)value, value));
   }
   

   Unfortunately you can't do where T : Enum although technically there is a work around for that.

   Similarly have a ParseEnumSelection which checks if the input is one of the enum constants.

3. There is nothing which really identifies the player - the class should have a Name property.

4. GiveResult should be renamed to RecordResult.

5. In your games class your rules definition uses ints to define which gesture beats which other gesture. Again this makes an implicit assumption about how your enums are numbered and in which order they are. So the tuples should be Tuple<Gesture, Gesture> rather than Tuple<int, int>

6. In Play you first have a switch on the result and the again you have comparisons to check for teh same thing again and a convoluted condition for printing which player won. If you move that logic up into the switch as well the code become easier to read and follow and you check the result only once:

    int result = WhoWon(p1move, p2move);
    switch (result)
    {
    case 0: 
    player1.GiveResult(Performance.Tied);
    player2.GiveResult(Performance.Tied);
    Console.WriteLine("It was a tie!");
    break;
    case 1: 
    player1.GiveResult(Performance.Won); 
    player2.GiveResult(Performance.Lost); 
    Console.WriteLine("Player 1 won, because {0}.", GetReason(p1move));
    break;
    case 2: 
    player1.GiveResult(Performance.Lost); 
    player2.GiveResult(Performance.Won); 
    Console.WriteLine("Player 2 won, because {0}.", GetReason(p2move));
    break;
    }
   }
   

Answer 2

one thing to start with.

I would get rid of your playAgain variable and replace the while statement like this

Original Code:

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);

(削除)

New Code:

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();
} while (result == 0);

I am pretty sure that you can use result because anything inside the loop is within the same scope as what is in the while clause as long as it is part of a do while statement.

I have been wrong before though (削除ここまで)

---

Even Newer Code

int result;
do
{
 Game.Play(me, computer);
 Console.WriteLine("Your scorecard: " + me.GetScoreCard());
 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);
 }
 Console.Clear();
} while (result == 0 || result == 1);

Here you have kept the same functionality and reduced the code to one loop.

• BenVlodgi: You can do this only if you declare result outside the loop, as in C# the scope closes before the while condition is checked. I thought about implementing it this way, but I just wanted to keep the menu somewhat separated from the main game loop. that is the only reason I even used a playAgain bool.
• Malachi: if you did it this way you would eliminate an entire variable, but you would be controlling two do while loops with the same variable. maybe it should be restructured a different way...@BenVlodgi
• Malachi: thank you @Olivier I Copy Pasted a lot of the answer...lol
• BenVlodgi: @Oliver While yes this does reduce it down to one loop, i also will automatically replay the game if the user chooses to clear their score, which may not be intended. In the original original code it was 2 separate questions, I combined it all into a menu which would not progress until an option was chosen, except when clear score was chosen.
• Mathieu Guindon: The reviewer gets reviewed? I like that!!

Answer 3

After some criticisms on my Main method, I've re-written it to the following. This implementation starts the user off at the main menu, which is easily scaled. To add a menu option, simply add the text to the gameMenu array, and add a corresponding action to take place in the switch statement. This implementation avoids multiple loops and confusions about what exactly is going on.

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);
}

• Malachi: I do like that. it gives me a nice fuzzy feeling