adeena
adeena

Reputation: 4167

Why does it appear that my random number generator isn't random in C#?

I'm working in Microsoft Visual C# 2008 Express.

I found this snippet of code:

    public static int RandomNumber(int min, int max)
    {
        Random random = new Random();

        return random.Next(min, max);
    }

the problem is that I've run it more than 100 times, and it's ALWAYS giving me the same answer when my min = 0 and max = 1. I get 0 every single time. (I created a test function to run it - really - I'm getting 0 each time). I'm having a hard time believing that's a coincidence... is there something else I can do to examine or test this? (I did re-run the test with min = 0 and max = 10 and the first 50ish times, the result was always "5", the 2nd 50ish times, the result was always "9".

?? I need something a little more consistently random...

-Adeena

Upvotes: 21

Views: 7459

Answers (14)

Hoen
Hoen

Reputation: 1

Your range is not correct. The minValue is inclusive in the range and the maxValue is exclusive in the range(meaning it won't be included in the range). So that's why it returns only 0.

Another useful note: having a Random instance be created in the method is not ideal as it might get the same seed on calling. So instead i would say use:

static Random gen = new Random();

public static int RandomNumber(int minValue, int maxValue){
    return gen.Next(minValue, maxValue);
}

Upvotes: 0

Tom
Tom

Reputation: 11

You're misunderstanding the line "random.Next(min, max)". "min" is in the place of the lowest number allowed to be generated randomly. While "max" is in the place of the lowest number NOT allowed to be generated it is not in the place of the largest number allowed to be drawn. So when the line is random.Next(0, 1) you are basically only allowing 0 to be drawn.

Upvotes: 1

Fedor Alexander Steeman
Fedor Alexander Steeman

Reputation: 1561

I found a very simple, but effective way to generate random numbers by just taking the last two digits of the current datetime milliseconds:

   int seed = Convert.ToInt32(DateTime.Now.Millisecond.ToString().Substring(1, 2));
   int cnr = new Random(seed).Next(100);

It is crude, but it works! :-)

Of course it would statistically generate the same number every hundred times. Alternatively, you could take all three digits or concatenate with other datetime values like seconds or so.

Upvotes: 0

SLaks
SLaks

Reputation: 887453

You're always getting 0 because Random.Next returns integers. You need to call Random.NextDouble, which will return a number between 0 and 1. Also, you should reuse your Random instance, like this:

[ThreadStatic]
static Random random;
public static Random Random { 
    get {
        if (random == null) random = new Random();
        return random;
    }
}
public static int RandomInteger(int min, int max)
{
    return Random.Next(min, max);
}
public static double RandomDouble() //Between 0 and 1
{ 
    return Random.NextDouble();
} 

If you want cryptographically secure random numbers, use the RNGCryptoServiceProvider class; see this article

EDIT: Thread safety

Upvotes: 6

Stephen Martin
Stephen Martin

Reputation: 9645

Several posters have stated that Random() uses a seed based on the current second on the system clock and any other instance of Random created in the same second will have the same seed. This is incorrect. The seed for the parameterless constructor of Random is based on the tick count, or number of milliseconds since boot time. This value is updated on most systems approximately every 15 milliseconds but it can vary depending on hardware and system settings.

Upvotes: 0

Greg Beech
Greg Beech

Reputation: 136627

This is an addendum to any answers, as the answer to this specific question is the bounds should be (0, 2) not (0, 1).

However, if you want to use a static wrapper method, then you must remember that Random is not thread-safe, so you either need to provide your own synchronization mechanism or provide a per-thread instance. Here is a largely non-blocking implementation which uses one generator to seed each per-thread generator:

public static class ThreadSafeRandom
{
    private static readonly Random seed = new Random();

    [ThreadStatic]
    private static Random random;

    public static int Next(int min, int max)
    {
        if (random == null)
        {
            lock (seed)
            {
                random = new Random(seed.Next());
            }
        }

        return random.Next(min, max);
    }

    // etc. for other members
}

Upvotes: 1

mateusza
mateusza

Reputation: 5743

random = new Random();

This initiates random number generator with current time (in sec). When you call your function many times before system clock changed, the random number generator is initiated with the same value so it returns same sequence of values.

Upvotes: 32

Chris Doggett
Chris Doggett

Reputation: 20757

As others have mentioned, the Random being built multiple times per second uses the same second as the seed, so I'd put the Random constructor outside your loop, and pass it as a parameter, like this:

public static int RandomNumber(Random random, int min, int max)
{
    return random.Next(min, max);
}

Also as mentioned by others, the max is exclusive, so if you want a 0 or 1, you should use [0,2] as your [min,max], or some larger max and then do a binary AND with 1.

public static int RandomOneOrZero(Random random)
{
    return random.Next(0, int.MaxValue) & 1;
}

Upvotes: 1

Alex Martelli
Alex Martelli

Reputation: 881675

Besides the 0-1 issue already noted in other answers, your problem is a real one when you're looking for a 0-10 range and get identical results 50 times in a row.

new Random() is supposed to return a random number with a seed initialized from the timer (current second), but apparently you're calling this code 50 times a second. MSDN suggests: "To improve performance, create one Random to generate many random numbers over time, instead of repeatedly creating a new Random to generate one random number.". If you create your random generator once outside the method, that should fix your "non-randomness" problem as well as improving performance.

Also consider this post for a better pseudo-random number generator than the system-supplied one, if you need "higher quality" pseudo-random numbers.

Upvotes: 3

Eric
Eric

Reputation: 95133

Don't create a wrapper method for Next. It wastes cycles creating a new instance of the Random class. Just use the same one!

Random myRand = new Random();

for(int i = 0; i < 10; i++)
{
    Console.WriteLine(myRand.Next(0, 10).ToString());
}

That should give you ten random values.

As has been said--Random is pseudo-random (as all implementations are), and if you create 100 instances with the same seed, you'll get 100 instances of the same results. Make sure that you're reusing the class.

Also, as folks have said, beware that MinValue is inclusive and MaxValue is exclusive. For what you want, do myRand.Next(0, 2).

Upvotes: 19

Dave Bauman
Dave Bauman

Reputation: 9678

That overload of Next() returns:

A 32-bit signed integer greater than or equal to minValue and less than maxValue; that is, the range of return values includes minValue but not MaxValue. If minValue equals maxValue, minValue is returned.

0 is the only possible value for it to return. Perhaps you want random.NextDouble(), which will return a double between 0 and 1.

Upvotes: 7

Annath
Annath

Reputation: 1262

The problem with min = 0 and max = 1 is that min is inclusive and max is exclusive. So the only possible value for that combination is 0.

Upvotes: 47

AgileJon
AgileJon

Reputation: 53586

The min is inclusive, but the max is exclusive. Check out the API

Upvotes: 6

Jason
Jason

Reputation: 52523

in VB i always start with the Randomize() function. Just call Randomize() then run your random function. I also do the following:

Function RandomInt(ByVal lower As Integer, ByVal upper As Integer) As Integer
    Return CInt(Int((upper - lower + 1) * Rnd() + lower))
End Function

Hope this helps! :)

Upvotes: -2

Related Questions