HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Rock-Paper-Scissors-Lizard-Spock Challenge, take 2

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
scissorspaperchallengespocktakerocklizard

Problem

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

I had a gut feeling that I was somehow abusing IComparable, @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;

Solution

-

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.

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


-

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:

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


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

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


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

Code Snippets

Console.WriteLine("Rock-Paper-Scissors-Lizard-Spock 1.1\n{0}\n", new string('=', 40));
Console.WriteLine("Rock-Paper-Scissors-Lizard-Spock 1.1");
Console.WriteLine(new string('=', 40));
Console.WriteLine();
var selections = Enum.GetValues(typeof(Selection));
_selectionsMap = Enumerable.Range(1, selections.Length)
    .Zip(selections,
         (index, selection) => new KeyValuePair<int, Selection>(index, selection));
new GameRule(Selection.Scissors, "cuts", Selection.Paper)
new GameRule(Selection.Scissors, "decapitates", Selection.Lizard)

Context

StackExchange Code Review Q#36426, answer score: 8

Revisions (0)

No revisions yet.