RPSLSMB OOP Version 2

Rock Paper Lizard Spock Monkey Banana

My original post

Based off of Malachi's post

I've changed the way rules work in this version to allow ties to result in double loss or double win. While statistically picking monkey wins against the computer, my intention is to make this a multi-player game which would require more strategy than the completely flat RPS or RPSLS.

I'm looking for any criticism/advice/review on the following code. I think the Rules structure is pretty complex and daunting.

Dictionary,   // A sub-dictionary of all of the gestures this one beats, and a string of why.
    Dictionary    // A sub-dictionary of all of the gestures this one loses to, and a string of why.
>>

I've considered moving it to a separate class. However, it does work perfectly fine as is, so I haven't.

Also you should note that the rules dictionary does contain a reason for why gestures lose, this isn't used in the current implementation, but I do plan to have it used when I have people playing in multi-player to be notified of "why they lost"... which is only slightly different from "why the other guy won".

Here is the full, runnable dump

```
class Program
{
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.WriteL

It might be simpler to define rules like this ...

List> Rules = new List> {
    Tuple.Create(Gesture.Rock,Gesture.Scissors,"Crushes","Get Crushed By"),
    ... etc ...
};

... because ...

• It's simpler (less "daunting")

• Easier to read (triplets of winning gesture, losing gesture, reason)

• Easier to e.g. load from a flat configuration/resource file.

• Less error-prone (you define everything twice, once for the winner and once for the loser; what you forget to define half the rule? where do you verify that you haven't forgotten?)

You could convert such a structure into a dictionary of dictionaries at load-time, e.g. if you wanted the improved performance of a dictionary instead of scanning a list.

The mapping between gameMenu options and integers isn't obvious unless you read the code; and the code is scattered (a gameMenu definition, a switch statement, Utils.PrintMenu, and Utils.PromptForRangedInt methods); perhaps instead have a UserInput class ...

static class UserInput
{
    internal enum Choice { Play, Reset, Quit };
    static Choice PromptUserAndGetChoice() { etc. }
}

... because then everything would be co-located in one class.

I like to put default: throw NotImplementedException(); at the end of switch statements.

Instead of ...

return "[Wins: " + Wins + "] [Loses " + Loses + "] [Ties " + Ties + "]";

... perhaps ...

return string.Format("[Wins: {0}] [Loses {1}] [Ties {2}]", Wins, Loses, Ties);

... because I find it easier to read the format of the latter output string.

Instead of abstract Player with two subclasses, you could have a concrete player class which takes the abstraction as a delegate parameter passed into its constructor

Func GetMove;
Player(Func getMove)
{
    this.GetMove = getMove;
}

It's worth having subclasses when there are two abstract methods, but when there's just one it's less code (only one class instead of three) to use a delegate.

WhatHappensToMe is called twice and checks the rules twice. Instead, what happens to me is the complement of what happens to them; if you define the enum values as -1, 0, and 1 then you could simply negate the value.

GetReason checks the rules yet again. Instead, maybe looking-up the rule should return the result and the reason at the same time.

StackExchange Code Review Q#44577, answer score: 5