11
\$\begingroup\$

This post is following-up on Rock-Paper-Scissors-Lizard-Spock Challenge

I had a gut feeling that I was somehow abusing IComparable<T>, @svick's answer confirmed that.

As @dreza was posting his answer I was in the process of proceeding with exactly that refactoring: stuffing the SelectionBase class with most of the functionality, effectlvely turning the derived classes into more or less useless, combersome objects that would be best implemented as an enum.

Building on @ChrisWue's answer, I came up with the following code:

Selections

Turned into enums:

public enum Selection
{
    Rock,
    Paper,
    Scissors,
    Lizard,
    Spock
}

GameRule

Designed to be represented as a string that looks exactly how Sheldon enumerates them:

public class GameRule
{
    private readonly Selection _winner;
    private readonly Selection _loser;
    private readonly string _verb;

    public GameRule(Selection winner, string verb, Selection loser)
    {
        _winner = winner;
        _loser = loser;
        _verb = verb;
    }

    public Selection Winner { get { return _winner; } }
    public Selection Loser { get { return _loser; } }

    public override string ToString()
    {
        return string.Format("{0} {1} {2}", _winner, _verb, _loser);
    }
}

GameResult

Rather than going static all the way, I chose an Evaluate factory method with a private constructor, and instead of having a bool that tracks whether the player wins, I've included the player's selection as a parameter of that constructor. The InvalidOperationException thrown in the ToString method is never hit, but it satisfies the compiler's need for all paths to return a value while eliminating a "default" else branch that would make a result be implicitly defined:

public class GameResult
{
    private readonly GameRule _outcome;
    private readonly Selection _player;

    private GameResult(GameRule outcome, Selection player)
    {
        _outcome = outcome;
        _player = player;
    }

    public override string ToString()
    {
        if (_outcome == null) return "Meh. Tie!";
        if (_player == _outcome.Winner) return string.Format("{0}. You win!", _outcome);
        if (_player == _outcome.Loser) return string.Format("{0}. You lose!", _outcome);

        throw new InvalidOperationException();
    }

    public static GameResult Evaluate(IEnumerable<GameRule> rules, Selection player, Selection sheldon)
    {
        var rule = rules.FirstOrDefault(r => r.Winner == player && r.Loser == sheldon || r.Winner == sheldon && r.Loser == player);
        return new GameResult(rule, player);
    }
}

This invited refactoring my Game class to look like this:

public class Game
{
    private readonly IUserInputProvider _consoleInput;
    private readonly IResultWriter _resultWriter;
    private readonly IEnumerable<GameRule> _rules;

    public Game(IUserInputProvider console, IResultWriter resultWriter, IEnumerable<GameRule> rules)
    {
        _consoleInput = console;
        _resultWriter = resultWriter;
        _rules = rules;
    }

    public void Run()
    {
        while (true)
        {
            LayoutGameScreen();

            var player = GetUserSelection();
            if (player == null) return;

            var sheldon = Selection.Spock;
            var result = GameResult.Evaluate(_rules, player.Value, sheldon);

            _resultWriter.OutputResult(result);
            Pause();
        }
    }

    private void Pause()
    {
        Console.WriteLine("\nPress <ENTER> to continue.");
        Console.ReadLine();
    }

    private void LayoutGameScreen()
    {
        Console.Clear();
        Console.WriteLine("Rock-Paper-Scissors-Lizard-Spock 1.1\n{0}\n", new string('=', 40));

        foreach (var item in Enum.GetValues(typeof(Selection)))
            Console.WriteLine("\t[{0}] {1}", (int)item + 1, item);

        Console.WriteLine();
    }

    private Selection? GetUserSelection()
    {
        var playable = Enum.GetValues(typeof(Selection));
        var values = playable.Cast<int>()
                             .Select(item => (item + 1).ToString())
                             .ToList();
        values.Add(string.Empty); // allows a non-selection

        var input = _consoleInput.GetValidUserInput("Your selection?", values);
        if (input == string.Empty)
        {
            var confirm = _consoleInput.GetValidUserInput("Giving up already? (Y/N)", new[] { "Y", "N" });
            if (confirm.ToUpper() == "Y") return null;
        }

        int selection;
        var isValidSelection = int.TryParse(input, out selection) && playable.Cast<int>().Contains(selection - 1);
        if (!isValidSelection) return GetUserSelection();

        return playable.Cast<Selection>().Single(item => (int)item == selection - 1);
    }
}

One thing I'm not sure I like, is the +1/-1 and the casting going on here, in order to display 1-based menu items; I refuse to explicitly assign an int value to each possible Selection.

The refactoring has greatly simplified the ConsoleResultWriter class:

public interface IResultWriter
{
    void OutputResult(GameResult outcome);
}

public class ConsoleResultWriter : IResultWriter
{
    public void OutputResult(GameResult outcome)
    {
        Console.WriteLine("\n\tSheldon says: \"{0}\"\n", outcome);
    }
}

...and left the Main method looking like this:

    static void Main(string[] args)
    {
        var consoleReader = new ConsoleUserInputProvider();
        var consoleWriter = new ConsoleResultWriter();
        var rules = new[] {
                            new GameRule(Selection.Scissors, "cuts", Selection.Paper),
                            new GameRule(Selection.Paper, "covers", Selection.Rock),
                            new GameRule(Selection.Rock, "crushes", Selection.Lizard),
                            new GameRule(Selection.Lizard, "poisons", Selection.Spock),
                            new GameRule(Selection.Spock, "smashes", Selection.Scissors),
                            new GameRule(Selection.Scissors, "decapitates", Selection.Lizard),
                            new GameRule(Selection.Lizard, "eats", Selection.Paper),
                            new GameRule(Selection.Paper, "disproves", Selection.Spock),
                            new GameRule(Selection.Spock, "vaporizes", Selection.Rock),
                            new GameRule(Selection.Rock, "crushes", Selection.Scissors)
                        };

        var game = new Game(consoleReader, consoleWriter, rules);
        game.Run();
    }

AI aside, can it get any better? Is there anything weird still going on? How's the naming? Any KISS-shredding interfaces?

\$\endgroup\$
8
\$\begingroup\$
  1. I chose an Evaluate factory method with a private constructor

    I think that GameResult.Evaluate() doesn't logically make much sense, this logic belongs to a method in Game, which means the constructor of GameResult would be public. Also, why does GameResult contain Selection player? Something like bool playerWon would be enough.

  2. Console.WriteLine("Rock-Paper-Scissors-Lizard-Spock 1.1\n{0}\n", new string('=', 40));
    

    I think this would be cleaner as several separate WriteLines:

    Console.WriteLine("Rock-Paper-Scissors-Lizard-Spock 1.1");
    Console.WriteLine(new string('=', 40));
    Console.WriteLine();
    
  3. One thing I'm not sure I like, is the +1/-1 and the casting going on here, in order to display 1-based menu items

    I agree. I think a better solution would be to create a structure that maps numbers to Selections. Dictionary is probably not a good idea, because you want to output the selections in order and Dictionary is unordered. Instead, you can use for example List<KeyValuePair<int, Selection>>:

    var selections = Enum.GetValues(typeof(Selection));
    _selectionsMap = Enumerable.Range(1, selections.Length)
        .Zip(selections,
             (index, selection) => new KeyValuePair<int, Selection>(index, selection));
    

    You could use this structure both in LayoutGameScreen() and GetUserSelection().

  4. new GameRule(Selection.Scissors, "cuts", Selection.Paper)
    new GameRule(Selection.Scissors, "decapitates", Selection.Lizard)
    

    These are incorrect grammatically, they should be "cut" and "decapitate".

\$\endgroup\$
4
\$\begingroup\$

Minor addition to @svick's answer:

If you change your enum to

public enum Selection
{
    Rock = 1,
    Paper,
    Scissors,
    Lizard,
    Spock
}

Then you have it one-based and don't need to do the +1/-1 dance.

\$\endgroup\$
  • 3
    \$\begingroup\$ I don't like this much, because I think it ties the model too tightly with presentation. \$\endgroup\$ – svick Dec 1 '13 at 3:50

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.