6
\$\begingroup\$

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);
            }
        }
    }
}
\$\endgroup\$
7
  • 1
    \$\begingroup\$ I don't have expertise for a proper review but I am almost certain you should be using an interface for the Player subclasses rather than holding references to them using the base class. The point of abstract base class is to implement common functionality while interface is to represent object confirming to an expected set of functionality. \$\endgroup\$ – the VBE - it's right for me May 9 at 12:42
  • 1
    \$\begingroup\$ @theVBE-it'srightforme Thanks for your comments. I used an abstract Player base class because I wanted derived classes to (1) have the same basic properites and behaviour and (2) be interchangeable in my GameManager and ScoreManager classes (since both you and the computer can play the game). Therefore, I'm not sure I fully understand the advantage of using a Player interface in this case. \$\endgroup\$ – ImNotThatCSharp May 9 at 13:14
  • 1
    \$\begingroup\$ Interface defines the set of methods/features/whatever that client code (eg GameManager and ScoreManager) can expect an object to conform to, so it more purely achieves the interchangeability aspect. In the conceptual framework of separating concerns, the concern of the interface would be to define what the object looks like externally, while the concern of the abstract base class would be to contain the common properties/functionality. \$\endgroup\$ – the VBE - it's right for me May 9 at 13:47
  • 2
    \$\begingroup\$ Naturally this means they can be used together; in your case just declare public interface IPlayer with same set of methods in Player, have Player : IPlayer, and change the references in the client classes from Player to IPlayer. \$\endgroup\$ – the VBE - it's right for me May 9 at 13:50
  • 1
    \$\begingroup\$ honestly on second thought I can probably be of at least some help trying to review it so will make attempt tomorrow \$\endgroup\$ – the VBE - it's right for me May 11 at 10:42
3
\$\begingroup\$

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 WinsAgainst method 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 a params argument.

    Lastly, the bool Beats(Move) method makes your ScoreManager.UpdateGameScore method 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 Rules class. Any logic in that class could be moved in to GameManager or ScoreManager.

\$\endgroup\$
7
  • \$\begingroup\$ Thanks, Greg. A lot of good suggestions here - I agree coupling is definitely something I need to work on reducing. Static constructors also seem very handy! Why do you have a getter on what looks like a private field, defeatedMoves? \$\endgroup\$ – ImNotThatCSharp May 13 at 9:35
  • \$\begingroup\$ @ImNotThatCSharp: The getter returns an IEnumerable<Move> which does not allow you to add items to the collection. This is in case you want to loop over the moves that this move defeats. The private field is a List<Move>, which allows you to add or remove items from the collection. We want to restrict who can decide which moves are defeated to enforce the business rules of your game (rock beats scissors and lizzard). \$\endgroup\$ – Greg Burghardt May 13 at 11:37
  • \$\begingroup\$ Reading the link about naming classes with "Manager" suffix I think it would be fair to say it that it is still acceptable to do so as a placeholder while someone is in the earlier stages of the project? I mean if it serves some useful purpose for them making sense of what they are trying to do they shouldn't obsess about finding a better name at that moment because they know they aren't "supposed" to name things that way. \$\endgroup\$ – the VBE - it's right for me May 13 at 22:54
  • 1
    \$\begingroup\$ @theVBE-it'srightforme: The earlier stages of a project are where names and coding idioms are established, and these features spread throughout the code. If you cannot come up with a good name, it might indicate a design problem with the class, or a need to understand the problem domain better. To be fair, this is a stage we all go through, but this should not be a stage we keep going through. Temporary code becomes permanent all too often. \$\endgroup\$ – Greg Burghardt May 13 at 23:20
  • 1
    \$\begingroup\$ If you're curious, that link in my previous comment was to the Apollo 11 lunar landing module code. The comment on that line of code reads # TEMPORARY, I HOPE HOPE HOPE and here we are 60 years later reading a "temporary" line of code. :) \$\endgroup\$ – Greg Burghardt May 13 at 23:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.