How to generate a random number in the constructor

Question

I know that it is not a good practice to call a method in the constructor of a class in C# but I stuck on something strange. My problem is that when I create an object of my class I need to assign a field in my object with a random number.

for instance

class RandomNumberHandler
    {
        private int randomNumber;
        public RandomNumberHandler()
        {
            this.randomNumber = GenerateRandomNumber();
        }

        private int GenerateRandomNumber()
        {
            return (new Random()).Next(3000) + 1000;
        }
    }

In my case I need a four digit number. I thought of generating the random number in the class where I am creating the object and passing it as a parameter to the constructor but generating a random number in the other class does not seem a very good idea either because I am trying to achieve strong cohesion for my classes. I am doing this for a "High quality code" course in my university and I am looking for the best approach. Any ideas how to do this are welcome :)

Answer 1

This code will work okay - except that you'll likely get the "same" random number if you call it in quick succession.

You could, of course, easily work around that by using a static random number (with a lock for thread safety if you're using multiple threads), such as:

class RandomNumberHandler
{
    private static Random random = new Random();
    private static object syncObj = new object();

    private int randomNumber;
    public RandomNumberHandler()
    {
        this.randomNumber = GenerateRandomNumber();
    }

    private static int GenerateRandomNumber()
    {
        lock(syncObj)
            return random.Next(3000) + 1000;
    }
}

Answer 2

First off: there is nothing wrong with calling non-virtual methods in the constructor. Where did you read that there was? (Note: calling virtual methods can be a problem; it is not an automatic no-no, but you need to watch what you are doing very carefully).

As an aside, it seems wasteful to generate a new Random instance every time GenerateRandomNumber is called. You can extract the Random instance to a field to fix that:

class RandomNumberHandler
{
    private readonly Random random = new Random();
    private int randomNumber;

    public RandomNumberHandler()
    {
        this.randomNumber = GenerateRandomNumber();
    }

    private int GenerateRandomNumber()
    {
        return this.random.Next(3000) + 1000;
    }
}

But this raises another question: if GenerateRandomNumber is only called once in each instance's lifetime (in the constructor), then it doesn't make sense to create a new Random for each object. So the next logical step is to make random be static. This means that GenerateRandomNumber can also become static (and indeed, it has to):

class RandomNumberHandler
{
    private static readonly Random Random = new Random();
    private int randomNumber;

    public RandomNumberHandler()
    {
        this.randomNumber = GenerateRandomNumber();
    }

    private static int GenerateRandomNumber()
    {
        return Random.Next(3000) + 1000;
    }
}

Answer 3

I don't see any cohesion there by just wrapping the Random class inside of another class named RandomHandler. Personally, I think thats awkward. If you need a brand new completely random number then just call Random().Next(3000) or whatever inside of the constructor like you said.

Answer 4

Random number generator only generating one random number

Short answer: You should use same Random instance across all Next() calls.

Answer 5

If you lift the Random instance as a static field and make the GenerateRandomNumber static, you can call it in the declaration of the randomNumber field:

class RandomNumberHandler {
  private static Random random = new Random();
  private int randomNumber = GenerateRandomNumber();

  private static int GenerateRandomNumber() {
    return random.Next(3000) + 1000;
  }
}

Or more simply (and less readable):

class RandomNumberHandler {
  private static Random random = new Random();
  private int randomNumber = random.Next(3000) + 1000;
}

Although it doesn't look like you're calling a method in a constructor, if you look at the generated CIL, you'll find out that you are.

Also, if you care about thread-safety, take a look at this article.