3

I use the method to generate unique number but I always get the same number -2147483648. Even if I stop the program, recompile it and run again I still see the same number.

  public static int GetRandomInt(int length)
    {           
        var min = Math.Pow(10, length - 1);
        var max = Math.Pow(10, length) - 1;
        var random = new Random();
        return random.Next((int)min, (int)max);
    }
Robert Harvey
  • 178,213
  • 47
  • 333
  • 501
Tomas
  • 17,551
  • 43
  • 152
  • 257
  • I think you may have a truncation error. -2147483648 is the lowest possible int. – amccormack May 16 '11 at 08:39
  • 5
    This is not a duplicate question, the issue is of a different kind – mmix May 16 '11 at 09:05
  • This article has been discussed hundreds of times. [FAQ from stackoverflow on this](https://stackoverflow.com/questions/767999/random-number-generator-not-working-the-way-i-had-planned-c) – Mikant May 16 '11 at 08:39

8 Answers8

7

Try externalizing the random instance:

private readonly Random _random = new Random();

public static int GetRandomInt(int length)
{           
    var min = Math.Pow(10, length - 1);
    var max = Math.Pow(10, length) - 1;
    return _random.Next((int)min, (int)max);
}
Darin Dimitrov
  • 1,023,142
  • 271
  • 3,287
  • 2,928
  • Doesnt Random use the time as a default seed? Why would this help? – LiamB May 16 '11 at 08:39
  • 1
    @Pino, yes, but if you call this method in a loop you will probably seed the random generator with the same value. Externalizing the Random variable ensures that it is seeded only once and everytime you call `.Next` on it you will get a random number. – Darin Dimitrov May 16 '11 at 08:43
  • @Pino: See my answer. By not creating a new `Random` instance every time you allow the RNG sequence to progress and actually return its pseudo-random values. If you re-initialize (re-seed) it every time you will get the same result until the system clock advances. (E: too slow :)) – SirViver May 16 '11 at 08:45
  • 2
    @SirViver @Darin, but OP said that he got the same number even after he recompiled. Wouldn't that eliminate the chance that he used the same time seed? – amccormack May 16 '11 at 08:55
3

This is not an issue of not reusing Random instance, the results he gets should be random on multiple starts, not always being -(2^32)

This is the issue with length being too big, and casting powers of length to int. If you break the code into following lines:

        var min = Math.Pow(10, length - 1);
        var max = Math.Pow(10, length) - 1;
        var random = new Random();
        var a = (int)min;
        var b = (int)max;
        return random.Next(a, b);

You'll see that a and b are -2147483648, making that the only possible result of Next(min, max) (the doc specifies if min==max, return min).

The largest length you can safely use with this method is 9. For a length of 10 you'll get System.ArgumentOutOfRangeException, for length > 10 you'll get the -2147483648 result.

mmix
  • 6,057
  • 3
  • 39
  • 65
  • Right, it is this overflow problem _and_ the instance thing – H H May 16 '11 at 10:09
  • Well, you have no basis to make that claim on this question, he might be doing this as a once off operation in which case it is actually a bad practice to keep a live random instance around. I can't believe you all closed this question. Have you even read it? – mmix May 16 '11 at 10:21
  • Yes, it was a little hasty. But the dupe question comes by several times/week here. The OP should have spelled out the range thing better. – H H May 16 '11 at 10:23
  • 2
    The way I see it the OP didn't know he has a range problem, maybe junior. IMHO, what is happening to this topic is very indicative of the blind rat-race that is going on in SO in few high volume tags such as c# and linq. It's all about points and few bother about anything else, apparently including senior members. Do you even realize that the highest ranked answer here does not even compile as it references instance field from a static method? The fact that dupe questions come along is no excuse for mistreatment of this question. Has SO jumped the shark? – mmix May 16 '11 at 10:47
2

You should keep an instance of Random and not new() it up all the time, that should give you better results.

Also check for what length actually is. It may be giving you funny results as to the limits.

flq
  • 22,247
  • 8
  • 55
  • 77
2

You have three problems with your code.

  1. You should externalize your random variable.
  2. You have a problem with truncation error.
  3. The range between min and max is way to large.

The first problem is because you may not have enough time to advance the seed when reinitializing your random variable. The second error comes from truncating your (what would b very large) numbers down to ints. Finally, your biggest problem is your range between your min and your max. Consider finding the range between min and max (as defined in your code) with inputs 1->20:

length  max-min        
1       8
2       89
3       899
4       8999
5       89999
6       899999
7       8999999
8       89999999
9       899999999
10      8,999,999,999
11      89999999999
12      899999999999
13      8999999999999
14      89999999999999
15      899999999999999
16      9E+15
17      9E+16
18      9E+17
19      9E+18

And keep in mind that the maximum integer is 2,147,483,647, which is passed on any number greater than 9.

amccormack
  • 13,207
  • 10
  • 38
  • 61
1

I think the problem is the calculation of min and max. They will be greater than Int32.MaxValue pretty fast...

Daniel Hilgarth
  • 171,043
  • 40
  • 335
  • 443
0

In your class, have one instance of Random, e.g.:

public class MyClass
{
private readonly Random random = new Random();

public static int GetRandomInt(int length)
{
  var min = Math.Pow(10, length - 1);
  var max = Math.Pow(10, length) - 1;
  return random.Next((int)min, (int)max);
}

}

The fact that random always returns the same values only exists for testing purposes.

Christian
  • 4,345
  • 5
  • 42
  • 71
0

Random classes usually use a seed to initialize themselves, and will usually return the same sequence provided the seed is the same one :

  • Always reuse the same Random() instance instead of recreating one over and over again
  • if you want unpredictable results, use a time-dependent seed rather than an hard-coded one

It's very difficult to code a truly random number generator. Most methods use external entropy generators (such as mouse movement, cpu temperature, or even complex physical mechanisms such as helium balloons colliding one another...).

Brann
  • 31,689
  • 32
  • 113
  • 162
0

The Random instance should be created only once and then reused. The reason for this is that the RNG is by default seeded with the current system time. If you rapidly create new Random instances (and pull one value from it) then many of them will be seeded with the same timestamp, because the loop probably executes faster than the system clock advances.

Remember, a RNG initialized by seed A will always return sequence B. So if you create three Random instances all seeded with for example 123, these three instances will always return the same number on the same iteration.

SirViver
  • 2,411
  • 15
  • 14