Problem
I would be grateful for a code review of my first “real” C# project (https://github.com/michaeljsiena/RockPaperScissors). It’s a basic rock, paper, scissors console game. It’s probably quite over-engineered for what it is, but I wanted to try applying some aspects of object-oriented design I’ve been learing in a simple console app.
I would especially appreciate comments on how I could further separate concerns in many of my methods. I also noticed much of the code involved in handling inputs is repeated so was hoping for some tips on how to reduce this.
Edit: I’ve already implemented theVBE-it’srightforme suggestion – thanks again!
Program
using System;
namespace RockPaperScissors
{
class Program
{
static void Main(string[] args)
{
Console.Title = "ROCK, PAPER, SCISSORS, LIZARD, SPOCK";
var gameManager = GameManager.Instance;
gameManager.PlayGame();
}
}
}
GameManager
using System;
namespace RockPaperScissors
{
// Uses a singleton pattern
public sealed class GameManager
{
private GameMode gameMode;
private IPlayer player1, player2;
private readonly ScoreManager scoreManager = ScoreManager.Instance;
private static GameManager _instance = null;
public static GameManager Instance
{
get => _instance ??= new GameManager();
}
public void PlayGame()
{
// game loop
do
{
// set up game
gameMode = SetGameMode();
InitialisePlayers(gameMode, ref player1, ref player2);
scoreManager.SetWinningScore();
// play game
int round = 1;
while (player1.Score < scoreManager.WinningScore && player2.Score < scoreManager.WinningScore)
{
ConsoleHelper.PrintColorTextLine($"nROUND {round}", ConsoleColor.Blue);
Rules.ShowMoveOutcomes();
// make moves
player1.MakeMove();
player2.MakeMove();
// update and showround information
scoreManager.UpdateGameScore(player1, player2);
scoreManager.ShowGameScore(player1, player2);
round++;
}
// show end of game information
scoreManager.ShowWinner(player1, player2);
scoreManager.ShowGameScore(player1, player2);
}
while (WillPlayAgain());
static GameMode SetGameMode()
{
ConsoleHelper.PrintColorText("GAME MODE ('1' = vs Computer, '2' = Simulation)?: ", ConsoleColor.Blue);
int playerInput;
while (!int.TryParse(Console.ReadLine(), out playerInput) || !(playerInput == 1 || playerInput == 2))
{
ConsoleHelper.PrintColorTextLine("Invalid input, try again...", ConsoleColor.Red);
}
return (GameMode)playerInput;
}
static void InitialisePlayers(GameMode gameMode, ref IPlayer player1, ref IPlayer player2)
{
switch (gameMode)
{
case GameMode.HumanVsComputer:
// get player name
ConsoleHelper.PrintColorText("PLAYER NAME: ", ConsoleColor.Blue);
string playerName = Console.ReadLine();
player1 = new HumanPlayer(playerName);
player2 = new ComputerPlayer("HAL 9000");
break;
case GameMode.ComputerVsComputer:
player1 = new ComputerPlayer("Skynet");
player2 = new ComputerPlayer("HAL 9000");
break;
}
}
static bool WillPlayAgain()
{
ConsoleHelper.PrintColorText("nPLAY AGAIN ('1' = yes, '2' = no)?: ", ConsoleColor.Blue);
int playerInput;
while (!int.TryParse(Console.ReadLine(), out playerInput) || !(playerInput == 1 || playerInput == 2))
{
ConsoleHelper.PrintColorTextLine("Invalid input, try again...", ConsoleColor.Red);
}
return playerInput == 1;
}
}
}
}
ScoreManager
using System;
using System.Collections.Generic;
namespace RockPaperScissors
{
// Uses a singleton pattern
public sealed class ScoreManager
{
private static ScoreManager _instance = null;
public static ScoreManager Instance
{
get => _instance ??= new ScoreManager();
}
private ScoreManager() { }
public int WinningScore { get; private set; }
public int PlayerScore { get; private set; }
public int ComputerScore { get; private set; }
public void SetWinningScore()
{
ConsoleHelper.PrintColorText("WINNING SCORE: ", ConsoleColor.Green);
int winningScore;
while (!int.TryParse(Console.ReadLine(), out winningScore) || winningScore < 1)
{
ConsoleHelper.PrintColorTextLine("Invalid input, must be a positive whole number...", ConsoleColor.Red);
}
WinningScore = winningScore;
}
public void UpdateGameScore(IPlayer player1, IPlayer player2)
{
if (player1.Move == player2.Move)
{
player1.Score += 1;
player2.Score += 1;
}
else if (player2.Move == Rules.MoveOutcomes[player1.Move].losingMove1 || player2.Move == Rules.MoveOutcomes[player1.Move].losingMove2)
{
player1.Score += 1;
}
else
{
player2.Score += 1;
}
}
public void ShowGameScore(IPlayer player1, IPlayer player2)
{
ConsoleHelper.PrintColorTextLine($"{player1.Name}'s Score: {player1.Score}n{player2.Name}'s Score: {player2.Score}", ConsoleColor.Green);
}
public void ShowWinner(IPlayer player1, IPlayer player2)
{
string message = (player1.Score == player2.Score)
? $"{player1.Name} and {player2.Name} tie!"
: $"{(player1.Score > player2.Score ? player1.Name : player2.Name)} wins!";
ConsoleHelper.PrintColorTextLine("n" + new string('*', message.Length), ConsoleColor.Green);
ConsoleHelper.PrintColorTextLine(message, ConsoleColor.Green);
ConsoleHelper.PrintColorTextLine(new string('*', message.Length), ConsoleColor.Green);
}
}
}
IPlayer
namespace RockPaperScissors
{
public interface IPlayer
{
public string Name { get; }
public Move Move { get; }
public int Score { get; set; }
public void MakeMove();
}
}
Player
namespace RockPaperScissors
{
public abstract class Player : IPlayer
{
public string Name { get; private set; }
public Move Move { get; protected set; }
public int Score { get; set; }
protected Player(string name) => Name = name;
public abstract void MakeMove();
}
}
HumanPlayer
using System;
namespace RockPaperScissors
{
public sealed class HumanPlayer : Player
{
public HumanPlayer(string name) : base(name) { }
public override void MakeMove()
{
ConsoleHelper.PrintColorText($"{this.Name}'s move: ", ConsoleColor.White);
int playerInput;
while (!int.TryParse(Console.ReadLine(), out playerInput) || !Enum.IsDefined(typeof(Move), playerInput))
{
ConsoleHelper.PrintColorTextLine("Invalid input, please try again...", ConsoleColor.Red);
}
Move = (Move)playerInput;
}
}
}
ComputerPlayer
using System;
namespace RockPaperScissors
{
public sealed class ComputerPlayer : Player
{
public ComputerPlayer(string name) : base(name) { }
public override void MakeMove()
{
Move = RandomMoveGenerator.GenerateRandomMove();
Console.WriteLine($"{this.Name} made a {this.Move}");
}
}
}
RandomMoveGenerator
using System;
using System.Linq;
namespace RockPaperScissors
{
public static class RandomMoveGenerator
{
private readonly static Random random = new Random();
private static readonly Move[] moves = Enum.GetValues(typeof(Move))
.Cast<Move>()
.ToArray();
public static Move GenerateRandomMove() => moves[random.Next(moves.Length)];
}
}
ConsoleHelper
using System;
// Made this to eliminate some of the clutter in GameManager.PlayGame()
namespace RockPaperScissors
{
public static class ConsoleHelper
{
public static void PrintColorText(string text, ConsoleColor color)
{
Console.ForegroundColor = color;
Console.Write(text);
Console.ResetColor();
}
public static void PrintColorTextLine(string text, ConsoleColor color)
{
Console.ForegroundColor = color;
Console.WriteLine(text);
Console.ResetColor();
}
}
}
GameMode
namespace RockPaperScissors
{
public enum GameMode
{
HumanVsComputer = 1,
ComputerVsComputer // simulation, mainly for testing
}
}
Move
namespace RockPaperScissors
{
public enum Move
{
Rock = 1,
Paper,
Scissors,
Lizard,
Spock,
}
}
Rules
using System;
using System.Collections.Generic;
using System.Text;
namespace RockPaperScissors
{
public static class Rules
{
private static readonly Dictionary<Move, (Move losingMove1, Move losingMove2)> _moveOutcomes = new Dictionary<Move, (Move, Move)>
{
{ Move.Rock, (Move.Scissors, Move.Lizard)},
{ Move.Paper, (Move.Rock, Move.Spock)},
{ Move.Scissors, (Move.Paper, Move.Lizard)},
{ Move.Lizard, (Move.Paper, Move.Spock)},
{ Move.Spock, (Move.Rock, Move.Scissors)},
};
public static Dictionary<Move, (Move losingMove1, Move losingMove2)> MoveOutcomes
{
get => _moveOutcomes;
}
public static void ShowMoveOutcomes()
{
ConsoleHelper.PrintColorTextLine("nMOVES", ConsoleColor.Blue);
foreach (KeyValuePair<Move, (Move losingMove1, Move losingMove2)> moveOutcome in Rules.MoveOutcomes)
{
ConsoleHelper.PrintColorTextLine($"{moveOutcome.Key} (key: '{(int)moveOutcome.Key}') beats {moveOutcome.Value.losingMove1} and {moveOutcome.Value.losingMove2}", ConsoleColor.DarkGray);
}
}
}
}
Solution
Overall a very good first attempt at a “real” project.
-
Drop “Manager” class name suffix — Consider renaming GameManager to simply Game. Similarly rename ScoreManager to something like GameScore. I see the “Manager” suffix frequently applied to classes that do not really manage something. Read I shall call it… Something manager for more information about why the “Manager” suffix isn’t the greatest.
-
Unnecessary use of singletons — there is no need to restrict then number of instances for GameManager and ScoreManager. If you need one and only one, consider using dependency injection or
newing up a ScoreManager in the constructor of GameManager. Sometimes objects only need to be singletons within a certain scope, and restricting classes to a single instance using a private constructor gains you nothing. -
Refactor nested ternary expression — the code to set the game winning message is impossible to read because a ternary expression is embedded in another ternary expression. It becomes a jumbled blob of text. Either Refactor into an if-else block or move this logic into its own method.
-
Decouple writing to console — since static classes are used to write to the console, you cannot isolate the critical classes in your game for unit testing. Consider defining an interface with the methods and properties required in order to interact with the console.
-
Define a class for moves — Believe it or not there is some behavior associated with a move. The logic of comparing moves to see which ones win belongs in its own class. It’s funny that my second suggestion was to not use singletons, but a Move class is honestly a good use case for singletons. As a bonus, this implementation also lets you practice using static constructors.
public class Move { public static readonly Move Rock = new Move("Rock"); public static readonly Move Paper = new Move("Paper"); public static readonly Move Scissors = new Move("Scissors"); public static readonly Move Spock = new Move("Spock"); public static readonly Move Lizzard = new Move("Lizzard"); public string Name { get; } private List<Move> defeatedMoves { get; } public IEnumerable<Move> DefeatedMoves => defeatedMoves; private Move(string name) { Name = name; defeatedMoves = new List<Move>(); } static Move() { Rock.WinsAgainst(Scissors, Lizzard); Paper.WinsAgainst(Rock, Spock); Scissors.WinsAgainst(Paper, Lizzard); Lizzard.WinsAgainst(Paper, Spock); Spock.WinsAgainst(Rock, Scissors) } private void WinsAgainst(params Move[] defeatedMoves) { this.defeatedMoves.AddRange(defeatedMoves); } public bool Beats(Move opponentMove) { return defeatedMoves.Contains(opponentMove); } }The line beginning
static Move() { ... }is a static constructor. This gets invoked the first time a static member of the Move class gets accessed. This constructor wires together the moves that defeat each other. By using access modifiers, singletons and a static constructor the compiler restricts who defeats who.Notice that the
WinsAgainstmethod is private, yet the static constructor in this class is still able to call this instance method, because the static constructor is a member of the Move class. More bonus points for practice using aparamsargument.Lastly, the
bool Beats(Move)method makes yourScoreManager.UpdateGameScoremethod much easier to read. No more parsing through a Dictionary object trying to infer the logic. The code reads exactly what it does:public void UpdateGameScore(IPlayer player1, IPlayer player2) { if (player1.Move == player2.Move) { player1.Score += 1; player2.Score += 1; } // else if (player2.Move == Rules.MoveOutcomes[player1.Move].losingMove1 || player2.Move == Rules.MoveOutcomes[player1.Move].losingMove2) else if (player1.Move.Beats(player2.Move)) { player1.Score += 1; } else { player2.Score += 1; } }This leaves very little need for the
Rulesclass. Any logic in that class could be moved in to GameManager or ScoreManager.