Understanding classes and using Random

Question

I've written the following class to return a random number like rolling a dice:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;

namespace GameTest
{
    class Dice
    {
    public int publicMinNum
    {
        get { return _minNum; }
        set { _minNum = value; }
    }

    public int publicMaxNum
    {
        get { return _maxNum; }
        set { _maxNum = value; }
    }

    static int _minNum;
    static int _maxNum;

    static Random diceRoll = new Random();
    public int rolled = diceRoll.Next(_minNum, _maxNum);
 }
}

This class is called a couple of times in my form:

    private void btnPushMe_Click(object sender, EventArgs e)
    {
        Dice myRoll = new Dice();
        myRoll.publicMinNum = 1;
        myRoll.publicMaxNum = 7;

        lblMain.Text = myRoll.rolled.ToString();

        Dice mySecondRoll = new Dice();
        mySecondRoll.publicMinNum = 1;
        mySecondRoll.publicMaxNum = 13;

        lblMain2.Text = mySecondRoll.rolled.ToString();
    }

As you can see, I call the class twice as myRoll and mySecondRoll. I thought by doing this it would create separate instances of the class and output two separate numbers (one between 1 and 6, the other 1 and 12)

The problems I am having are:

1) the first number out is always 0.

2) the two instances of the class interfere with one another, ie. the number that should be between 1 and 6 just isn't.

I'm wondering, not just how to fix the code, but would also like an explanation of what is happening here and why, thanks.

Answer 1

The problem is that you are declaring the fields in the Dice class as static. This means that there will only be one instance of that variable, which will be shared across all instances of the class within the application.

The following line:

public int rolled = diceRoll.Next(_minNum, _maxNum);

... gets run the moment you create your new Dice(), which means that you haven't yet initialized your _minNum and _maxNum values yet: that's why it's giving you a 0. You could turn this into a property, so the code would wait to be run until you asked for it:

public int Rolled { get { return diceRoll.Next(_minNum, _maxNum); } }

... but typically properties are not expected to change just by asking for their value. This sort of code tends to create so-called Heisenbugs, which are very difficult to track down because the system's behavior changes simply by trying to observe it.

So here's one way you might re-write your class, using a Roll() method to actually perform the roll, and a property that allows code to keep checking on the last roll's value whenever necessary:

public class Die
{

    // Using a constructor makes it obvious that you expect this
    // class to be initialized with both minimum and maximum values.
    public Die(int minNum, int maxNum)
    {
        // You may want to add error-checking here, to throw an exception
        // in the event that minNum and maxNum values are incorrect.

        // Initialize the values.
        MinNum = minNum;
        MaxNum = maxNum;

        // Dice never start out with "no" value, right?
        Roll();
    }

    // These will presumably only be set by the constructor, but people can
    // check to see what the min and max are at any time.
    public int MinNum { get; private set; }

    public int MaxNum { get; private set; }

    // Keeps track of the most recent roll value.
    private int _lastRoll;

    // Creates a new _lastRoll value, and returns it.
    public int Roll() { 
        _lastRoll = diceRoll.Next(MinNum, MaxNum);
        return _lastRoll;
    }

    // Returns the result of the last roll, without rolling again.
    public int LastRoll {get {return _lastRoll;}}

    // This Random object will be reused by all instances, which helps
    // make results of multiple dice somewhat less random.
    private static readonly Random diceRoll = new Random();
}

(note that "die" is the singular form of "dice"). Usage:

private void btnPushMe_Click(object sender, EventArgs e)
{
    Die myRoll = new Die(1, 7);
    lblMain.Text = myRoll.Roll().ToString();

    Die myRoll2 = new Die(1, 13);
    lblMain2.Text = mySecondRoll.Roll().ToString();
}

Answer 2

Question Two is already aswered: because the variables are static:

static int _minNum;
static int _maxNum;

Question One on the other hand isnt answered yet, so here goes:

public int rolled = diceRoll.Next(_minNum, _maxNum);

this is not some dynamic call. This is a field initialisation, and will be set even before the constructor. You can check this out by debugging through the dice the first time.

at that point both _minNum and _maxNum are still 0, so rolled will be set to 0

this can be fixed by turning rolled into a property too:

    public int rolled
    {
        get { return diceRoll.Next(_minNum, _maxNum); }
    }

At the moment _minNum and _maxNum are getting set the first time because they are static, therefor when you create the second dice, they are already set.

Edit, since a recommendation was asked, this is how I'd create it:

The dice

class Dice
{
    private static Random diceRoll = new Random();

    private int _min;
    private int _max;
    public int Rolled { get; private set; }

    public Dice(int min, int max)
    {
        _min = min;
        _max = max;

        // initializes the dice
        Rolled = diceRoll.Next(_min, _max);
    }

    public int ReRoll
    {
        get
        {
            Rolled = diceRoll.Next(_min, _max);
            return Rolled;
        }
    }
}

Note the the dice has two properties: Rolled, and ReRoll. Because your intention is unclear, I've added both to illustrate the behavior.

Rolled is set by the constructor. If you want a new number, you can ReRoll.

If you do intentionally wanted the lifetime of a roll to be one per dice (but I dont think so) You'd remove the ReRoll method.

The dice would be called like this:

    private static void Main(string[] args)
    {
        Dice myRoll = new Dice(1, 7);

        // All the same
        var result1 = myRoll.Rolled.ToString();
        var result2 = myRoll.Rolled.ToString();
        var result3 = myRoll.Rolled.ToString();

        // something new
        var result4 = myRoll.ReRoll.ToString();

        Dice mySecondRoll = new Dice(1, 13);
        var result = mySecondRoll.ReRoll.ToString();
    }

Answer 3

I would change your class to look more like the following:

class Dice
{
  // These are non-static fields. They are unique to each implementation of the
  // class. (i.e. Each time you create a 'Dice', these will be "created" as well.
  private int _minNum, _maxNum;

  // Readonly means that we can't set _diceRand anywhere but the constructor.
  // This way, we don't accidently mess with it later in the code.
  // Per comment's suggestion, leave this as static... that way only one
  // implementation is used and you get more random results.  This means that
  // each implementation of the Dice will use the same _diceRand
  private static readonly Random _diceRand = new Random();

  // A constructor allows you to set the intial values.
  // You would do this to FORCE the code to set it, instead
  // of relying on the programmer to remember to set the values
  // later.
  public Dice(int min, int max)
  {
    _minNum = min;
    _maxNum = max;
  }

  // Properties
  public Int32 MinNum
  {
    get { return _minNum; }
    set { _minNum = value; }
  }

  public Int32 MaxNum
  {
    get { return _maxNum; }
    set { _maxNum = value; }
  }

  // Methods
  // I would make your 'rolled' look more like a method instead of a public
  // a variable.  If you have it as a variable, then each time you call it, you
  // do NOT get the next random value.  It only initializes to that... so it would
  // never change.  Using a method will have it assign a new value each time.
  public int NextRoll()
  {
    return _diceRand.Next(_minNum, _maxNum);
  }    
}

Answer 4

Your get/setter backing fields are marked as "static". If a variable is declared "static", the value is persisted throughout the application and shared between different instances of the type they live in.

See here.

Also,

since your class properties contain no logic, I suggest using "automatic" properties.

    class Dice
    {
      public int publicMinNum { get; set; }
      public int publicMaxNum { get; set; }
      Random diceRoll = new Random();
      public int rolled = diceRoll.Next(publicMinNum , publicMaxNum );
    }

tutorial on automatic properties here.

Answer 5

Your problem is due to the static members.

From the MSDN documentation on static, "While an instance of a class contains a separate copy of all instance fields of the class, there is only one copy of each static field."

Answer 6

I think the real problem here is that you haven't quite modeled a Die properly.

A die has a min and max value (that define the start and end of a range) but once a die has been made you are not able to change this i.e. a six sided die isn't made into an eight sided die. As such there is no need for the public setters.

Now, not all die share the same range, this is something that is specific to each die and so these properties should belong to the instance and not be static.

Again each die object has a CurrentRoll value representing the number that is face up and this is indeed generated at random. However to change the CurrentRoll of a die you need to Roll it.

This leaves an implementation of Die looking a something like

class Die
{
    private static Random _random;

    public int CurrentRoll { get; private set; }

    public int Min { get; private set; }

    public int Max { get; private set; }

    public Die(int min, int max)
    {
        Min = min;
        Max = max;
        Roll();
    }

    public int Roll()
    {
        CurrentRoll = _random.Next(Min, Max+1); // note the upperbound is exlusive hence +1
        return CurrentRoll;
    }
}

and you would use it like

public static void Main()
{
    Die d1 = new Die(1, 6);
    Die d2 = new Die(1, 6);

    Console.WriteLine(d1.Roll());
    Console.WriteLine(d2.Roll());
    //...
}

demo