8
\$\begingroup\$

I've been writing code for nearly 40 years now and am still not too old to learn and understand new things. Right now, my focus is a bit of OO with functional programming combined and C# with Linq is an excellent way to fool around with this.
I'm also practicing deferred execution and aggregating to make it really interesting. So I designed a dice roller. And to start, I begin by specifying various dice types:

public enum DiceType { D2 = 2, D3 = 3, D4 = 4, D6 = 6, D8 = 8, D10 = 10, D12 = 12, D20 = 20, D100 = 100 }

Very basic, actually. Each enum value just indicates the number of sides a die has. These values are very common, with the D2 (coin flip) and D6 (regular die) being the most well-known.
Next is a static class that can basically roll any dice, and will do this until infinity:

public static class Dice
{
    public static IEnumerable<int> Roll()
    {
        var rnd = new Random();
        while (true) { yield return rnd.Next(1, 7); }
    }

    public static IEnumerable<int> Roll(this DiceType die)
    {
        var rnd = new Random();
        while (true) { yield return rnd.Next((int)die) + 1; }
    }

    public static IEnumerable<int> RollX(this IEnumerable<int> rolls, int x = 3)
    {
        while (true) { yield return rolls.Take(x).Sum(i => i); }
    }
}

The Roll() method will be the stereotype die with six sides. Hardcoded, as someone might change the value for D6 to 8 and cause problems. This method will always roll a six-sided die.
But Roll(this DiceType die) is more interesting as it basically extends the DiceTyper enumeration. And it will roll that specific die into infinity so if I use DiceType.D20.Roll() then I will get an endless list of values between 1 and 20...
The third method RollX() is an even more interesting one. It will use the previous dice enumerator and take an X amount of values to sum them, before taking the next X amount of dice. By default, it will roll 3 times. And DiceType.D6.Roll().RollX(5) will roll 5 dice, each resulting in a value between 1 and 6, and add them to get values between 5 and 30, and a bell curve.
Next, two clock-related aggregators that will limit the amount of time allowed to keep rolling:

public static IEnumerable<T> MaxTime<T>(this IEnumerable<T> source, long totalMs)
{
    var sw = new Stopwatch();
    foreach (var item in source)
    {
        if (!sw.IsRunning) { sw.Restart(); }
        if (sw.ElapsedMilliseconds < totalMs) { yield return item; }
        else { yield break; }
    }
}

public static IEnumerable<T> MaxTicks<T>(this IEnumerable<T> source, long totalTicks)
{
    var sw = new Stopwatch();
    foreach (var item in source)
    {
        if (!sw.IsRunning) { sw.Restart(); }
        if (sw.ElapsedTicks < totalTicks) { yield return item; }
        else { yield break; }
    }
}

The stopwatch class can be found in System.Diagnostics.
The principle is simple: it will do a deferred execution until the time is up, which can either be a matter of seconds or a matter of clock ticks.
As a final step, I want an aggregator to just add all the results:

public static SumTotal Total(this IEnumerable<int> data)
{
    var result = new SumTotal();
    foreach (var i in data) { result.Add(i); }
    return result;
}

And this introduces this class:

public class SumTotal
{
    public long Sum { get; set; } = 0;
    public long Total { get; set; } = 0;
    public long Min { get; set; } = long.MaxValue;
    public long Max { get; set; } = long.MinValue;

    public SumTotal Add(long value)
    {
        Total++;
        Sum += value;
        if (Max < value) { Max = value; }
        if (value < Min) { Min = value; }
        return this;
    }

    public override string ToString() => $"For {Total:#,##0} items, total of {Sum:#,##0}, average of {Sum * 1.0 / Total:#,##0.00} between {Min:#,##0} and {Max:#,##0}.";
}

So I get a total containing the number of rolls, the minimum and maximum values rolled and a total of all values and can calculate the average value rolled.
Now, to use it, all I have to do is define a rolling queue like this:

public static IEnumerable<int> D12Queue = DiceType.D12.Roll();

This generates a static variable from where I can just take an X amount of rolls. Simply using D12Queue.Take(5) will give me 5 random values between 1 and 12. This makes it practical to use in games where you'd need to emulate a dice roll.
So, is this a practical solution? I'm just training and learning new skills so I haven't thought about that aspect...


The latter dice queue is what my main focus will be. I can use this:
var queue = DiceType.D6.Roll(); to generate a queue. And then:
var roll = queue.First(); which will give a new dice roll with every call.
var yahtzee = queue.Take(5); to roll 5 dice and see if I rolled a Yahtzee.
var highRoll = queue.RollX(5).Take(20).Max(); Which will roll 5 dice repeatedly for 20 times and return the highest value.
So basically, I made dice rolling part of Linq queries so I don't need to write loops if I need multiple rolls. I just take the number of required rolls from the queue.

\$\endgroup\$
  • 3
    \$\begingroup\$ I recently wrote a 41 part blog series on the subject of how to generalize the lessons of LINQ to representing randomness; you might find it interesting. Start here: ericlippert.com/2019/01/31/fixing-random-part-1 \$\endgroup\$ – Eric Lippert Nov 12 at 19:12
  • \$\begingroup\$ I'm considering if the DiceType enum should be converted into a list of int constants and extend the Roll() method to the int type instead. \$\endgroup\$ – Wim ten Brink Nov 13 at 10:55
1
\$\begingroup\$

Let me just repeat what @Ron Klein said, dont put your dices into enum, the real set of all possible dices is much greater than what your enum offers.

Let me show how I think the Dice class would be better implemented:

class Dice : IEnumerable<int>
{
    private int Sides;
    private int Shift;
    private Random Generator;

    public Dice(int sides, int shift, Random generator)
    {
        Sides = sides;
        Shift = shift;
        Generator = generator;
    }

    public Dice(int sides, int shift, int seed) : this(sides, shift, new Random(seed)) {}
    public Dice(int sides, int shift) : this(sides, shift, new Random()) {}
    public Dice(int sides, Random generator) : this(sides, 1, generator) {}
    public Dice(int sides) : this(sides, 1) {}
    public Dice(Random generator) : this(6, generator) {}
    public Dice() : this(6) {}

    public int Roll()
    {
        return Generator.Next(Sides) + Shift;
    }

    public IEnumerator<int> GetEnumerator()
    {
        while (true) yield return Roll();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

Now you can create whatever dice you want:

var d20 = new Dice(20);
var coin = new Dice(2);
var cube = new Dice(6);
var uncommon = new Dice(7);
var d0to5 = new Dice(6, 0);
var d15to30 new Dice(16, 15);

You can roll once

var roll = d20.Roll(); //1-20

Or you can roll N times

var sum = uncommon.Take(3).Sum(i => i); //3-21

Or you can even roll as many as the CPU can do in a certain amount of time/ticks

var rolls = cube.MaxTicks(250);

Also as mentioned by others, the Random class needs a seed and the default constructor seeds from current time. My Dice class implementation allow this default Random but also allows to inject Random instance seeded in a way out of scope of the Dice class.

var d20 = new Dice(20, new Random(myseed));

I have actualy added more constructors to simplify this and I have also added Shift property to the Dice as it seemd a waste to have +1 as constant, if it might be possible to have dices starting with zero or any other number.

var d20 = new Dice(20, 1, myseed);

Anyway I think that the MaxTick and MaxTime methods are quite controversial. One thing is they break SRP. Good rule of thumb though is that methods should not instantiate objects and do some work (other that the work needed to instantiate those objects). They should do one or the other, but not both. Other thing is I'm not sure they will fulfill their purpose as mentioned in comments under other answer.

\$\endgroup\$
  • \$\begingroup\$ Actually, no! I can use First() to get just one roll or Take() to get a number of rolls. By repeating calls to First(), I keep getting new rolls. The principle is that I defined a dice queue from where you can get as many dice rolls as you need. An infinite number of rolls to be used whenever needed. \$\endgroup\$ – Wim ten Brink Nov 13 at 10:29
  • \$\begingroup\$ Creating an infinite enumeration was my purpose here. It would be bad when someone called DiceType.D20.Roll().Last(); as it would run forever. You're missing the point, though. I implemented it as an enumeration so you can get a list of rolls like DiceType.D6.Roll().Take(5); would give 5 values immediately. Not the sum of 5 values. Useful for a Yahtzee game. \$\endgroup\$ – Wim ten Brink Nov 13 at 10:52
  • 1
    \$\begingroup\$ @WimtenBrink I'm not sure that would be a good idea to share one Random object. It is a very simple RNG and it shows some patterns if you for example generate coordinates with it (rolling 2 dice repeatedly one after the other is kinda the same). But Maybe it would be enough for your case. \$\endgroup\$ – slepic Nov 14 at 6:51
  • 1
    \$\begingroup\$ @WimtenBrink Also one shared static instance may be quite easy to guess what numbers it is going to generate based on the time at which you start the program. I've been thinking maybe you could have one static instance and use this to generate seeds for new Random instances that you will pass to each new Dice. This way it may still be a bit predictable, but the predictions will divert from real values very fast. \$\endgroup\$ – slepic Nov 14 at 6:52
  • 1
    \$\begingroup\$ @WimtenBrink another option might be to use a mre sofisticated RNG (like docs.microsoft.com/en-us/dotnet/api/…) then i guess it wouldn't be much of a problem to share just one instance of it... \$\endgroup\$ – slepic Nov 14 at 6:53
8
\$\begingroup\$

Random

Don't new up a new instance of Random for each and every call of your methods. The reason for this is that the default seed used in the constructor is not random. Usually it's based on the current time, or something similar. This means that if you call the method many times at once, the random number generators might provide the same numbers.

Instead, create a single static instance that's used by all calls.


public static SumTotal Total(this IEnumerable<int> data)
{
    var result = new SumTotal();
    foreach (var i in data) { result.Add(i); }
    return result;
}

The linq way of doing this is data.Aggregate(new SumTotal(), (item, aggregate) => aggregate.Add(item)).

\$\endgroup\$
  • 2
    \$\begingroup\$ That bug has been fixed in new versions of .NET, after only being in the platform for 19 years. But it is still a bad idea to make a new Random every time you need a new number. \$\endgroup\$ – Eric Lippert Nov 12 at 19:14
  • 1
    \$\begingroup\$ The New() is called just once for every dice queue. And the method is used to create one queue for every dice needed and grab values from the queue whenever needed. So, not a problem to me. \$\endgroup\$ – Wim ten Brink Nov 12 at 19:50
  • \$\begingroup\$ The Total method is an aggregator method, similar to Aggregate, ToList and a few other methods. Using aggregators and understanding how they work are different things. \$\endgroup\$ – Wim ten Brink Nov 12 at 20:00
  • 2
    \$\begingroup\$ @WimtenBrink the point still stands, and if you are providing this code as a library, you don't have control about how it is called, or how often. Best to think ahead and don't make the mistakes that are easy to prevent. \$\endgroup\$ – JAD Nov 12 at 20:04
  • \$\begingroup\$ If I do make a library out of it, it would include a static class with the various dice queues as static variables, depending on what it will be part off. The rest would basically become an internal class. This code by itself isn't meant to be a complete library. It needs more to be more practical. \$\endgroup\$ – Wim ten Brink Nov 12 at 20:23
5
\$\begingroup\$

Nice code.

Some comments :)

Too Defensive

You wrote:

The Roll() method will be the stereotype die with six sides. Hardcoded, as someone might change the value for D6 to 8 and cause problems. This method will always roll a six-sided die.

What if someone changes the underlying value of D2 to 3? Won't that be a major surprise?

My take about this approach is: don't write idiot proof code, otherwise you'd be chasing your tail. Write code that an average programmer could understand.

In your code, there's a convention that the die enum name has to be the same as its underlying int value. That's a design decision you took. It's a valid design decision. Stick to it.

In addition, if, for some reason, you choose to change your implementation of the underlying enum value, you'll have to remember that the logic is also implemented, hard coded, in your Roll() method. In other words, you break DRY.

O/CP

When you have an enum in your code, it should be a well defined, closed group of values that usually share something. The "closed" part is essential.

Take weekdays, for instance. There won't be any additional weekday. There's no eighth value that can be added. Therefore, we can have them as an enum.

Take card suites. There are only four of them. I think my point is clear.

When we declare an enum, we say something like: "these are the only values of this group."

However, in your code, you declare (only) the following values as possible die/dice:

2, 3, 4, 6, 8, 10, 12, 20, 100

What if someone would want to have your cool engine to be applied on a card suite? It's 13 cards in a suite. And you don't have 13 as a value in your enum.

In programming, there's a very important principle named Open/Closed Principle. I think that your enum is not aligned with O/CP, because if you'd want to use your random engine on a "die" with 13 values, then you'd have to add D13 to your enum, rebuild and redeploy your solution. When you're aligned with O/CP - you provide a way to extend your program ("open to extensions") without rebuilding it ("closed to modifications").

So maybe there's no need to wrap those values in an enum in the first place? Simply write a class that takes the number of edges the "die" has in its constructor, and you're done.

It's not as "pretty" as an enum, but it sure is more extensible.

Deferred Execution

You write:

The principle is simple: it will do a deferred execution until the time is up, which can either be a matter of seconds or a matter of clock ticks.

I'm not sure what you mean by that. It's not that the code runs on a different thread. Maybe I'm missing something because I haven't been coding in C# for a long while, but this code sure seems to be blocking.

I think that the added value of those methods (MaxTime and MaxTicks) is low, and it's pretty much up to the consuming developer to implement them as needed.

In addition to all that, there's sometime a confusion between wall-clock time (which is how things are implemented in your code) and processing time. In a multi-threaded program (or in a multi-processed system), your program, or your executing thread, is sometimes paused so that other processed/threads are executed.
Having this point in mind, you might end up executing far less iteration than expected.
Example: usually you get 10000 iterations in 300 ticks, but in a busy system you get only 500 under the same 300 ticks. The numbers are imaginary, but you get the idea.
For doing the actual measurements, you can read this.

Again, I think that adding those measurements to your code somehow increases complexity and have a low value.

Calculating Min/Max

It's mostly a matter of style, but I tend to use the SDK's provided functions when possible.
In your code:

if (Max < value) { Max = value; }

Can be replaced with:

Max = Math.Max(Max, value)

To me, this is easier to understand the intention of the code using the built-in Math.Max() method.

Cheers :)

\$\endgroup\$
  • \$\begingroup\$ Really? Math.Max instead avoiding unnecesary write? I mean c=math.max(a,b) why not. But a=math.max(a,b) why yes? \$\endgroup\$ – slepic Nov 13 at 5:55
  • \$\begingroup\$ @slepic yes, IMO intention (or semantics), readability and consistency are stronger considerations than performance. There's a famous quote: Premature optimization is the root of all evil... If you profile your program and see that there's a performance issue - then (and only then) optimize your code. \$\endgroup\$ – Ron Klein Nov 13 at 6:02
  • \$\begingroup\$ I know what premature optimization is. But I would object that this isn't it.There is difference between premature optimization and awareness of performance aspects of your code (especialy in a lib it would be bad to ignore performance until some user of the lib reports performance issues, especialy if you could have seen it before you even started implementing it). And I would also object whether a less then + an assignment is less readable than Math.Max.Anyway I wouldn't have brought this up if you didn't bring this up. It was optimal and you suggest suboptimal for a little readability gain. \$\endgroup\$ – slepic Nov 13 at 6:38
  • \$\begingroup\$ Fair enough. We both have our opinions. I even like the debate :) \$\endgroup\$ – Ron Klein Nov 13 at 7:12
  • 1
    \$\begingroup\$ @WimtenBrink You can roll a 13-side dice and get a number from it, which corresponds to the card. In fact, you have D2, which is a coin flip (hey, you called it!). As far as I know, a dice and a coin are different. Just allowing to pass a different value (an integer) with the number of sides could be enough. This way, you can use both an enum and a different value. Also, here's a set of odd dice (3 to 15): amazon.com/Green-Unusual-Odd-Numbered-Dice/dp/B00YLWVOXU (source: reddit.com/r/rpg/comments/aroymo/…) \$\endgroup\$ – Ismael Miguel Nov 13 at 12: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.