0

I wrote this function : 

int getRandomNumber(int min,int max)
{
    srand(time(NULL));
    return rand()%(max-min+1);
}

and called it like this:

 int x=getRandomNumber(0,2);
 int y=getRandomNumber(0,2);

But I am always getting the same value for x and y i.e. x=y always.
How to fix it?

Sahil Sareen
  • 1,813
  • 3
  • 25
  • 40
  • I updated my answer and included an example of C++11 code that might help you. Please, have a look. – FreeNickname Dec 11 '13 at 07:39
  • 1
    Ye it can be quite helpful to flag it with either `c` or `c++` but not both, to get better tailored answers. – luk32 Dec 11 '13 at 07:40

3 Answers3

7

srand is meant to initialize the generator. You are supposed to initialize it once e.g. at the start of the program. Then call rand.

Now you are initializing it every time you call the function. The reason why it is the same is because you supply the same argument twice for srand. time(0) returns the number of seconds since epoch till current time.

Basically you restart the generator for each subsequent call to the same state in every second. If you let it run for a span of 3 seconds you would see 3 different groups of values.

Also your formula is slightly wrong. You forgot to shift it by min.

rand()%(max-min+1)+min

Also if you work with c++ you might take a suggestion from FreeNickname and use c++ specific features.

luk32
  • 15,812
  • 38
  • 62
  • Also the parameter is very important for srand(). The same seed makes the same sequence. – Polymorphism Dec 11 '13 at 07:27
  • I did that but now if I run a for loop like : srand(time(NULL)); for(int p=0;p<50;++p) { int x=.. int y=.. } x and y now get different values BUT THE SAME VALUE ALL THE TIME i.e FOR EACH ITERATION OF THE LOOP. – Sahil Sareen Dec 11 '13 at 07:28
  • @Polymorphism I edited the answer to give an exact explanation of observed reasons. – luk32 Dec 11 '13 at 07:34
  • @SahilSareen Well, I would need to see full code and set of results. Also please note that default `rand` is know to behave quite poorly for low modulos. It can be bizzare when you use `rand()%3`. – luk32 Dec 11 '13 at 07:38
2

The problem is that x and y are initialized almost instantly. So time(NULL) in both cases returns the same value. You can initialize it once, as luk32 suggested. But the better option is to avoid using rand(). There are new options in C++11, which are way better in terms of randomness quality and ease of use. Take a look at this presentation: rand considered harmful.

An example of code that you want (I didn't have an opportunity to test it though):

#include <random>

int getRandomNumber(int min,int max)
{
    std::random_device randomDevice; //I put it here just for simplification. Creating a random device is slow, so you should avoid creating it every time.
    std::mt19937 rgen(randomDevice()); // mersenne_twister
    std::uniform_int_distribution<int> uniformDistribution(min, max); // min and max are included
    return uniformDistribution(rgen);
}
FreeNickname
  • 7,398
  • 2
  • 30
  • 60
  • There is no guarantee that two different instances of `std::random_device` will produce distinct sequences of random numbers, so this code can suffer from **exactly** the same problem as the original version. – Pete Becker Dec 11 '13 at 14:10
  • @PeteBecker, you are probably right. It might be implementation-defined. Anyway, as I said, it is not recomended to recreate randomDevice every time, since it is expensive. I wanted to simplify the example, and it seems that I've gone a bit too far :) What do you think about making it static? I don't want to make it global. – FreeNickname Dec 13 '13 at 03:48
0

Rand is considered harmful Use new c++11 features.

Community
  • 1
  • 1
Rafał Dembek
  • 108
  • 1
  • 6
  • Tendentiously-named papers do not magically create good code. Aside from the inappropriate multiple calls to `stand()`, the original code works just fine. – Pete Becker Dec 11 '13 at 14:11