C# – Why Random Number Generator Isn’t Random

Every time you do new Random() it is initialized using the clock. This means that in a tight loop you get the same value lots of times. You should keep a single Random instance and keep using Next on the same instance.

//Function to get a random number 
private static readonly Random random = new Random(); 
private static readonly object syncLock = new object(); 
public static int RandomNumber(int min, int max)
{
    lock(syncLock) { // synchronize
        return random.Next(min, max);
    }
}

Edit (see comments): why do we need a lock here?

Basically, Next is going to change the internal state of the Random instance. If we do that at the same time from multiple threads, you could argue "we've just made the outcome even more random", but what we are actually doing is potentially breaking the internal implementation, and we could also start getting the same numbers from different threads, which might be a problem - and might not. The guarantee of what happens internally is the bigger issue, though; since Random does not make any guarantees of thread-safety. Thus there are two valid approaches:

• Synchronize so that we don't access it at the same time from different threads
• Use different Random instances per thread

Either can be fine; but mutexing a single instance from multiple callers at the same time is just asking for trouble.

The lock achieves the first (and simpler) of these approaches; however, another approach might be:

private static readonly ThreadLocal<Random> appRandom
     = new ThreadLocal<Random>(() => new Random());

this is then per-thread, so you don't need to synchronize.

My guess is that randomNumber creates a new instance of Random each time... which in turn creates a new pseudo-random number generator based on the current time... which doesn't change as often as you might think.

Don't do that. Use the same instance of Random repeatedly... but don't "fix" it by creating a static Random variable. That won't work well either in the long term, as Random isn't thread-safe. It will all look fine in testing, then you'll mysteriously get all zeroes back after you happen to get unlucky with concurrency :(

Fortunately it's not too hard to get something working using thread-locals, particularly if you're on .NET 4. You end up with a new instance of Random per thread.

I've written an article on this very topic which you may find useful, including this code:

using System;
using System.Threading;

public static class RandomProvider
{    
    private static int seed = Environment.TickCount;

    private static ThreadLocal<Random> randomWrapper = new ThreadLocal<Random>
        (() => new Random(Interlocked.Increment(ref seed)));

    public static Random GetThreadRandom()
    {
        return randomWrapper.Value;
    }
}

If you change your new Random() call to RandomProvider.GetThreadRandom() that will probably do everything you need (again, assuming .NET 4). That doesn't address testability, but one step at a time...