Dice game rules implementation

Question

My in-laws taught me a dice game a couple years ago, and we play every once in a while. A recent excellent answer from @radarbob inspired me to proceed with translating the rules of that dice game into code.

Here's what came out of it:

So it works perfectly, and somehow I'm just as unlucky with this virtual version as in the real-life one (had to click dozens of time to get a freakin' opening roll; by that time my mother-in-law already has thousands of points, every time). At least in this version I can inject some CrookedDie implementation if I want!

I'd like the CalculateRollScore method reviewed, to see what could be improved.

It turns out I thought the code was working as it should. So I wrote a couple unit tests and... well this is where not writing unit tests has bitten me.

Well I had to change the rules anyway, since the rule for 4x 1's was just my wife being mixed-up. My mother-in-law and I agree that 3x 1's is 1000, 4x 1's is 2000 and 5x 1's is 3000.

So before I show any code, I'll show this:

Now here's the working code (view original code here), with only minor changes that don't mootinize any already posted answer:

namespace DiceGame
{
    public interface IRollScoreRules
    {
        int CalculateRollScore(IEnumerable<IRollResult<int>> results);
    }

    public class GameRollScoreRules : IRollScoreRules
    {
        public virtual bool IsOpeningRoll(IEnumerable<IRollResult<int>> results)
        {
            return CalculateRollScore(results) >= 500;
        }

        public virtual int CalculateRollScore(IEnumerable<IRollResult<int>> results)
        {
            var score = 0;

            // if less than 3 1's were rolled, each rolled 1 is 100pts:
            score += results.GroupBy(e => e.Value)
                            .Where(g => g.Key == 1)
                            .Where(g => g.Count() < 3)
                            .Sum(g => g.Count() * 100);

            // if less than 3 5's were rolled, each rolled 5 is 50pts:
            score += results.GroupBy(e => e.Value)
                            .Where(g => g.Key == 5)
                            .Where(g => g.Count() < 3)
                            .Sum(g => g.Count() * 50);

            // if more than 3 of anything other than 1 were rolled, determine number of "extra dice":
            var extras = results.GroupBy(e => e.Value)
                                .Where(g => g.Key != 1 && g.Count() > 3)
                                .ToDictionary(kvp => kvp.Key, kvp => kvp.Count() - 3);

            var extraOnes = results.GroupBy(e => e.Value)
                                   .Where(g => g.Key == 1 && g.Count() > 3)
                                   .ToDictionary(kvp => kvp.Key, kvp => kvp.Count() - 3);

            // any triplet is 100x nominal value; each extra die is another 100x nominal value:
            score += results.GroupBy(e => e.Value)
                            .Where(g => (g.Key != 1 && g.Count() >= 3))
                            .Sum(g => (g.Key * 100) + (extras.ContainsKey(g.Key) ? extras[g.Key] : 0) * (g.Key * 100));

            //score += results.GroupBy(e => e.Value)
            //    .Where(g => (g.Key == 1 && g.Count() >= 3))
            //    .Sum(g => (g.Key * 100) + (extraOnes.ContainsKey(g.Key) ? extraOnes[g.Key] : 0) * (g.Key * 100));

            // 3x 1's is 1000x nominal value; each extra die is another 1000x nominal value:
            score += results.GroupBy(e => e.Value)
                            .Where(g => g.Key == 1 && g.Count() >= 3)
                            .Sum(g => (g.Key * 1000) + (extraOnes.ContainsKey(g.Key) ? extraOnes[g.Key] : 0) * (g.Key * 1000));

            return score;
        }
    }
}

Answer 1

Well, I am not familiar with die, but the obvious improvement is to encapsulate those rules into separate entities. For example:

public interface IRollScoreRule
{
    int CalculateRollScore(IEnumerable<IRollResult<int>> results);
}

public interface IRollScoreRules : IRollScoreRule
{
    bool IsOpeningRoll(IEnumerable<IRollResult<int>> results);
    void AddRule(IRollScoreRule rule);
    void RemoveRule(IRollScoreRule rule);
}

public class ScoreFivesRule : IRollScoreRule
{
    int CalculateRollScore(IEnumerable<IRollResult<int>> results)
    {
        // i think some of this logic can be extracted to base class
        // results.GroupBy(e => e.Value).Where(g => g.Key == 5) part for example
        // is common for most of the rules
        var score = results.GroupBy(e => e.Value)
                        .Where(g => g.Key == 5)
                        .Where(g => g.Count() < 3)
                        .Sum(g => g.Count() * 50);
        return score;
    }
}

Then in your GameRollScoreRules.CalculateRollScore method you can iterate through collection of rules (using IEnumerable.Aggregate, for example) to get the total score, instead of having one huge method.

I also think you might want to tweak the IsOpeningRoll method. I mean, if I understand the use-case from your screenshots correctly, then with every dice roll, you want to know both - score value and if this value is higher then 500. Which means that using your GameRollScoreRules class as it is you would essentially calculate score twice (by calling CalculateRollScore and IsOpeningRoll methods). I think you sould either pass the resulting score to IsOpeningRoll method as a parameter, or remove this method all together and replace CalculateRollScore return type with some complex object (which will contain score value and bool flag) instead.

Answer 2

Frankly speaking, if someone in my team wrote those LINQ statements, I would kill him. Using LINQ on this, you just complicated the simple task. I tried to reconstruct it using simple C#, but yet I am still not sure if it's correct.

    public virtual int CalculateRollScore(IEnumerable<IRollResult<int>> results)
    {
        var score = 0;
        var diceMap = (new int[7]).ToList();
        foreach (var item in results)
        {
            diceMap[item.Value]++;
        }

        // if less than 4 1's were rolled, each rolled 1 is 100pts:
        if (diceMap[1] < 4)
        {
            score += diceMap[1] * 100;
        }

        // if less than 3 5's were rolled, each rolled 5 is 50pts:
        if (diceMap[5] < 3)
        {
            score += diceMap[5] * 50;
        }

        int nominalValue = diceMap.FindIndex(i => i >= 3);
        int dieCount = nominalValue > 0 ? diceMap[nominalValue] : 0;
        // any triplet is 100x nominal value; each extra die is another 100x nominal value:
        if (nominalValue > 0)
        {
            int extra = dieCount > 3 ? (dieCount - 3) * nominalValue * 100 : 0;
            score += nominalValue * 100 + extra;
        }

        // 4x 1's is 1000x nominal value; each extra die is another 1000x nominal value:
        if (nominalValue == 1 && dieCount >= 4)
        {
            int extra1 = dieCount > 4 ? (dieCount - 4) * nominalValue * 1000 : 0;
            score += nominalValue * 1000 + extra1;
        }
        return score;
    }

As I tested my method, the score of the set { 4, 1, 1, 1, 1 } is not matched with your method. Your method returns 1100, but it should be 1200

• 100 for triplet of 1's
• 100 for extra 1
• 1000 for 4x of 1

Correct me if I'm wrong, as I'm not familiar with the rules at all.

Answer 3

I think you're over-thinking the solution

My first thought is that I'd like to iterate a Rule collection, and give each rule the dice roll results.

foreach(Rule rule in RulesCollection) {
    score =+ rule.calculate(results);
}

With this "wishful thinking" starting point this means all your rules derive, or implement, Rule (or whatever you want to call it). If in the final analysis every rule calculates a score for that specific dice combination then the above snippet should be achievable. If a given rule needs unique methods, fine; but for polymorphic purposes they all calculate so they can all implement (or inherit) Rule.

Dump the interfaces

interfaces implementing interfaces implemented by abstract classes. This is madness. If all this is coming from code to interfaces, not implementation "interface" can be an abstract class, interface, or even a non-abstract class. The "interface" means giving the client code a consistent set of methods & properties so every object can be handled the same. If there is only one class implementing an interface, that interface is likely unnecessary. If in the future you need one, make it then.

Encapsulate for Consistency, Clarity, and Expressiveness

There is clearly a need for the grouped dice. My initial thought is that this best fits in the Dice class. AND that data structure could be a class as well.

public class Dice {
    public DiceMap Map { get; private set; }

    public Dice (int diceCount) {
        //existing code

        DiceMap = new DiceMap(sidesCount);
    }

     public int Roll() {
         //did not show all original code.

         int temp;

         foreach (Die die in dice) {  //does this need to be a "for" ?
             temp = die.Roll();
             total =+ temp;
             DiceMap[temp-1]++;
         }
     }

     // calling before rolling returns zero, makes sense.
     public int HowMany(int count) { return groups.HowMany(count); }

     // yes, an inner class. Only Dice class needs it.
     // I'll admit this may be overkill but I want to illustrate the idea
     // of encapsulating - hiding structure (the array) while exposing
     // it's meaning - how many 1's? for example.

     // The other point is to "go deep with OO design", so to speak.

     public class DiceMap {
         int[] groups;

         public DiceMap (int count) { groups = new int[count]; }

         public int HowMany(int dieFace) {
             // check for dieFace out of array bounds

             return groups[dieFace-1];
         }
    }
}

now deep in some Rule class this:

if (myDice.HowMany(3) > 3) { // your code here }

or this, depending on what we decide to pass in

if (myDiceMap.HowMany(3) > 3 ) 

RuleCollection

Make a class and put those "Add" and "Remove" functions in there!

public class RuleCollection {
    protected List<Rule> rules;

    public RuleCollection (params Rule[] rules) {
        this.rules = new List<Rule>;
        foreach (Rule rule in Rules) { rules.Add(rule); }

    public void Add (Rule rule) { rules.Add(rule); }
    public bool Remove (Rule rule) { return rules.Remove(rule) }
     // List<T>.Remove() returns a bool.

    // assumes dice have been rolled
    public int Calculate(DiceMap diceMap) {
        int score = 0;

        foreach (Rule rule in rules) {
            score =+ rule.Calculate(diceMap);
        }
    }
}

OpeningRoleRule

Seems to me every Rule implements a piece of the overall score counting for a single roll of the dice. So the opening roll is just all those rules applied - see above - then checked against some minimum value. So maybe it's not a Rule per-se.

public class TheGameClass {
   protected RulesCollection rules;
   protected Dice dice;

   dice.Roll();
   if ( rules.Calculate(dice.Map) >= minimumOpeningScore ) { ... }
}

OO Change Goodness

If the number of dice sides does not change, I want to do this:

public class DiceMap {
   // new properties
   public int Ones { get { return groups[0]; }
   public int Twos { get { return groups[1]; }
   // and so on
   public int Sixes { get { return groups[5]; }
}

Notice all the dice game code we don't have to touch!