0

I wrote this class to generate a random code, it shouldn't create any two repetitive number. I want to know, in this code, how much it possible that we have collision?

 public string MyRandom()
 {
    Random r = new Random();
    int x = r.Next(1000);//Max range
    PersianCalendar pc = new PersianCalendar();
     string _myrandom = pc.GetMonth(DateTime.Now).ToString() + pc.GetDayOfMonth(DateTime.Now).ToString() + pc.GetHour(DateTime.Now).ToString() + pc.GetMinute(DateTime.Now).ToString() + pc.GetSecond(DateTime.Now).ToString() + pc.GetMilliseconds(DateTime.Now).ToString() + x.ToString();

    return _myrandom;
 }
Incredible
  • 3,495
  • 8
  • 49
  • 77
mosyflasher
  • 489
  • 1
  • 6
  • 16
  • 2
    See http://stackoverflow.com/questions/1654887/random-next-returns-always-the-same-values – L-Four Apr 07 '14 at 09:08
  • 1
    Collisions are _very_ likely if you call it in a loop. You need to cut that random variable out and paste it outside of the method or provide a method parameter for it. Side-note: i would store `DateTime.Now` in a variable, it's not so _cheap_ as someone might think. – Tim Schmelter Apr 07 '14 at 09:10
  • I don't use that class in the loop , this generator is using for naming photos which uploaded in my Application. – mosyflasher Apr 07 '14 at 09:23
  • I assumed because i use GetMilliseconds in that , we have never collision , it doesn't right? – mosyflasher Apr 07 '14 at 09:25
  • @user3117091 Well no. You are guaranteed a collision if you have more than one thread trying to save a file at the same time. Seriously read what L-Three posted. http://stackoverflow.com/questions/1654887/random-next-returns-always-the-same-values – Aron Apr 08 '14 at 05:15

3 Answers3

0

If we do

List<string> s = new List<string>();

for(int i = 0; i < 100; i++)
{
    s.Add(MyRandom());
}

Why? When random class is created in a loop, you can get duplicate random numbers. More Info

Solution: Create Random instance outside loop.

Community
  • 1
  • 1
Nikhil Agrawal
  • 47,018
  • 22
  • 121
  • 208
0

It is safer to use more stronger methods such as RNGCryptoServiceProvider which you can find several samples of its usage. Generally, pure random generation is not possible is very high concurrency loads, but reduce its probability to a reasonable amount.

Ali Dehghan
  • 462
  • 3
  • 6
0

Well first, because you are using the date in the way that you are, then there is only one possible millisecond per year where the _myrandom string could possibly be repeated.

Within that millisecond each year, the odds of hitting a random string you have already generated are down to the variable x, and related directly to how many other random strings you have made in that millisecond (and the previous milliseconds of each previous year). If, for example, you were to have made 10 random strings last year in that millisecond and 4 the year before, the chances of duplication this year are 14:999, which is a chance of about 1.4%.

Of course, if you make 1000 strings per millisecond, the chance of duplication is 100%.

In fact, the chances of duplication are slightly higher, due to the fact that the C# Random number generator doesn't produce a true random number. It's a pseudo-random number, so the chances of a collision are higher.

I see that you've written: "this generator is using for naming photos which uploaded in my application" in the comments above, which makes it unlikely that you'll be making 1000+ names per millisecond. Generally, whenever you are using a random number generator in this way, it's a good idea to just put in some error checking in 1 of two ways: 1. offensive or 2. defensive.

Offensive would be waiting for the error (two identical file names) to happen and then dealing with it (maybe running your random generator again until the error isn't thrown). Defensive would be to check all of the filenames in the file, or keep a list of them all, to check that your new filename isn't the same before you even try to save it. This is more watertight (because you aren't relying on an error to be thrown) but far more computationally expensive.

DIXONJWDD
  • 1,276
  • 10
  • 20
  • That is potentially much safer - yes. But again, if you are making more than 1 name per millisecond, then you could potentially have a collision. If you are **NEVER** going to **EVER** make 2 per millisecond, you don't even need the random number, but if you are potentially ever going to make 2 in a millisecond, I'd recommend that you perform some sort of check to prevent the potential error. – DIXONJWDD Apr 07 '14 at 09:44
  • I think Even i make 2 name per millisecond, that x variable will prevent collision or at least it will decrease that? – mosyflasher Apr 07 '14 at 09:53
  • It will decrease the chances of a collision massively, yes, but it will not prevent one. If you make 2 strings per millisecond, the chance of a collision is just above 0.1%. If there is **any** chance of a collision, I would recommend that you put error handling in or take preventative measures to check for a collision before attempting the save. – DIXONJWDD Apr 07 '14 at 09:57
  • so i can save generated _myrandom in an array and check it before every assigning to photos – mosyflasher Apr 07 '14 at 10:16
  • You can do that, or loop through the names of all the files currently in the directory to check for a collision. Of course all you actually need to do is check for a collision with all of the random numbers (x) generated that millisecond (this would be easier if you also added the year to the string, as you could then empty the array every millisecond and wouldn't need to store it for next year). Alternatively you can catch the "filename already exists" error and try making a new filename which doesn't exist. – DIXONJWDD Apr 07 '14 at 10:25
  • your solution was so useful, Thanks my friend – mosyflasher Apr 07 '14 at 15:24
  • No problem. If you feel this fully answers your question, could you accept the answer so as to mark the question as answered. – DIXONJWDD Apr 07 '14 at 18:50