C# Random Numbers aren't being "random"

I know that the C# Random class does not make "true random" numbers, but I'm coming up with an issue with this code:

    public void autoAttack(enemy theEnemy)
    {
        //Gets the random number
        float damage = randomNumber((int)(strength * 1.5), (int)(strength * 2.5));

        //Reduces the damage by the enemy's armor
        damage *= (100 / (100 + theEnemy.armor));

        //Tells the user how much damage they did
        Console.WriteLine("You attack the enemy for {0} damage", (int)damage);

        //Deals the actual damage
        theEnemy.health -= (int)damage;

        //Tells the user how much health the enemy has left
        Console.WriteLine("The enemy has {0} health left", theEnemy.health);
    }

I then call the function here (I called it 5 times for the sake of checking if the numbers were random):

        if (thePlayer.input == "fight")
        {
            Console.WriteLine("you want to fight");
            thePlayer.autoAttack(enemy1);
            thePlayer.autoAttack(enemy1);
            thePlayer.autoAttack(enemy1);
        }

However, when I check the output, I get the exact same number for each 3 function calls. However, each time I run the program, I get a different number (which repeats 3 times) like this:

 You attack the enemy for 30 damage.
 The enemy has 70 health left.

 You attack the enemy for 30 damage.
 The enemy has 40 health left.

 You attack the enemy for 30 damage.
 The enemy has 10 health left.

I will then rebuild/debug/run the program again, and get a different number instead of 30, but it will repeat all 3 times.

My question is: how can I make sure to get a different random number each time I call this function? I am just getting the same "random" number over and over again.

Here is the random class call that I used:

    private int randomNumber(int min, int max)
    {
        Random random = new Random();
        return random.Next(min, max);
    }

Solution 1:

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...

Solution 2:

You didn't show us the code for randomNumber. If it looks anything like

private int randomNumber(int m, int n) {
    Random rg = new Random();
    int y = rg.Next();
    int z = // some calculations using m and n
    return z;
}

Well, then there is your issue. If you keep creating new instances of Random, it's possible that they will sometimes have the same seed (the default seed is the system clock which has limited precision; create them quickly enough and they get the same seed) and then the sequence produced by this generator will always be the same.

To fix this, you have to instantiate an instance of Random once:

private readonly Random rg = new Random();
private int randomNumber(int m, int n) {
    int y = this.rg.Next();
    int z = // some calculations using m and n
    return z;
}

And to clear up another point, even if you do this, the output from Random is still not "true" random. It's only psuedorandom.