Rolling a dice with custom amount of faces

Question

So i am creating a dice using a public class called die. this class has 2 constructors and 3 other methods. When i call class within the main program, it will not print the desired (random) result.

Firstly i am trying to print out a random number between 1-6 based on the default values. Once i figure this out, i would eventually like to specify a number of sides for and print out a number between 1 and this specific number.

/// Represents one die (singular of dice) with faces showing values between
/// 1 and the number of faces on the die.
public class Die
{
    private int numFaces, faceValue;
    public Die() => (numFaces, faceValue) = (6, 1);

    public Die(int faces)
    {
        numFaces = faces;

        if (numFaces < 3) 
            numFaces = 6; 
        faceValue = 1; 
    }
    public void RollDie()
    {
        //for (int ctr = 0; ctr <= numFaces; ctr++)
        var rnd = new Random();
        for (int ctr = 1; ctr <= 20; ctr++)
            faceValue = rnd.Next(1, numFaces + 1);
    }
    public int GetFaceValue() => faceValue;
    public int GetNumFaces()  => numFaces;
}
public class Program
{
    public static void Main()
    {
        var myDie = new Die(1);
        for (int i =0; i < 20; i++) 
        {
            myDie.RollDie();
            Console.WriteLine(myDie.GetFaceValue());
        }
    }
}

You never _roll_ the die. Also, `RollDie` shouldn't take a parameter, your number of faces is already stored in a member variable. — tkausl, Sep 02 '18 at 19:16
And in addition to what @tkausl wrote, the RollDie should not create a local instance of the `Random` class. Instead, you should use a field for the `Random` class, so that you can roll the die in a loop and actually get random results every time. — Zohar Peled, Sep 02 '18 at 19:18
Some other tips: it is a good idea to not set default 6 when numFaces < 3 is provided. It is a *hidden behaviour*. You should throw a `ArgumentException` with description of the problem. Also, you shouldn't initialize with 1 - it is unexpected. You should either throw exception if it is unitialized, roll dice if unitialized or initialize in constructor. The last one thing: Better `numFaces` optional argument to avoid logic duplication. — Yeldar Kurmangaliyev, Sep 02 '18 at 19:24
Thanks, i've changed my code based of @tkausl advice. As for the other two suggestions, im not entirely sure how too attempt this — Tom Tok, Sep 02 '18 at 19:27
Creating a new `Random` each time you roll will undoubtedly result in unwanted repeats if you ever invoke in a tight loop. — Parrish Husband, Sep 02 '18 at 19:32
@ParrishHusband Indeed it is, I am unsure of a way around this unwanted repetition tho — Tom Tok, Sep 02 '18 at 22:54
Having well documented code is important, but that doesn't mean comment for the sake of commenting :P In nearly all the cases, you're comments we redundent and I can get the same information from reading the code itself — AustinWBryan, Sep 03 '18 at 06:36

Parrish Husband · Answer 1 · 2018-09-02T20:19:20.740

1

Since Random is time seeded when the parameterless constructor is used, this can have the very negative consequence of duplicating results.

Excerpt from the API documentation:

However, because the clock has finite resolution, using the parameterless constructor to create different Random objects in close succession creates random number generators that produce identical sequences of random numbers.

...

On most Windows systems, Random objects created within 15 milliseconds of one another are likely to have identical seed values.

This is a safer approach in regards to getting random numbers when creating and rolling multiple dice:

public class Die
{
    static int seed = Environment.TickCount;
    static readonly ThreadLocal<Random> DieRandom = new ThreadLocal<Random>(() 
        => new Random(Interlocked.Increment(ref seed)));

    public int FaceCount { get; }
    public int Value { get; private set; }

    public Die() : this(6) // default to 6 faces
    {
    }

    public Die(int faceCount)
    {
        if (faceCount < 3) // validate input
            throw new ArgumentOutOfRangeException(nameof(faceCount));

        this.FaceCount = faceCount;
    }

    public int Roll()
    {
        Value = DieRandom.Next(1, FaceCount + 1);
        return Value;
    }
}

Edit: updated the Random with thread safety as suggested here.

edited Sep 02 '18 at 20:19

answered Sep 02 '18 at 19:42

Parrish Husband

3,148
18
40

2

One important note, the `Random` class is not thread safe so if you are going to make the variable static you need a lock around the `Random.Next(1, FaceCount)` calling `Next` from multiple threads at the same time can break the class. Also, `Next` has a exclusive upper bound so it would need to be `.Next(1, FaceCount + 1);` or `.Next(0, FaceCount) + 1;`, if you don't do this passing in 6 would give you numbers only in the 1-5 range. – Scott Chamberlain Sep 02 '18 at 20:03
@ScottChamberlain thanks, that's a great point on thread safety. Also good spot on the bounds. – Parrish Husband Sep 02 '18 at 20:06
This seems like a much simpler approach. Unfortunately i am unable to change the structure of my code. I have updated my original post with my solution. I am now trying to figure out how to avoid repeated numbers when using the random method. I have set a loop to run the program 20 times and each time it results in the same number being returned. Any ideas on how to prevent this would be appreciated! – Tom Tok Sep 02 '18 at 23:02
@TomTok is this homework? Read the linked documentation I posted for the answer to your question. – Parrish Husband Sep 02 '18 at 23:04
Also if you're allowed to, look into how you could overload `Roll` to return multiple rolls. It's good practice anyway. – Parrish Husband Sep 02 '18 at 23:17
I have followed the links you have provided and have tried many solutions but i still cannot seem to figure it out. here is my current rolldice function: (edit) i will just update the original post too my current code – Tom Tok Sep 03 '18 at 00:06
@TomTok you can't create a new instance of the `Random` class every time your `RollDie` method is invoked. Think of where you could create it that only gets called once initially. – Parrish Husband Sep 03 '18 at 05:37

Rolling a dice with custom amount of faces

1 Answers1