1

In my c# code I have a static method. Here is the code sample: 

public class RandomKey
{
    public static string GetKey()
    {
        Random rng = new Random();
        char[] chars = new char[8];
        for (int i = 0; i < chars.Length; i++)
        {
            int v = rng.Next(10 + 26 + 26);
            char c;
            if (v < 10)
            {
                c = (char)('0' + v);
            }
            else if (v < 36)
            {
                c = (char)('a' - 10 + v);
            }
            else
            {
                c = (char)('A' - 36 + v);
            }
            chars[i] = c;
        }

        string key = new string(chars);
        key += DateTime.Now.Ticks.ToString();
        return key;
    }
}

I am calling this function from another Class's method.

Class SomeClass
{
    Void SomeMethod()
    {
        for(int i=0; i<100; i++)
        {
            System.Diagnostics.Debug.WriteLine(i + "===>" + RandomKey.GetKey());
        }
    }
}

But now the problem is sometimes I am getting the same output from the static method though the function was called separately. Is there any wrong with my code?

raisul
  • 640
  • 1
  • 5
  • 26
  • 1
    declare Random() outside the static Method – fubo Jun 09 '15 at 06:08
  • 4
    When you use the parameterless constructor for `Random()` it uses `Environment.TickCount` for the seed, which only changes every few milliseconds. Therefore if you call that too quickly you end up with consecutive instances constructed with the same seed, and hence the same sequence of random numbers. – Matthew Watson Jun 09 '15 at 06:09
  • @MatthewWatson : your comment is a better answer than posted answers. You should answer instead of commenting ;) – Falanwe Jun 09 '15 at 06:13
  • @falan The proper approach is closing as duplicate – CodesInChaos Jun 09 '15 at 06:22
  • @Falanwe: in what way do you think it's better? I've explained the problem in my answer the (with less details, I admit) and proposed a solution (that Matthew's comment didn't.). So, what makes it better? – Zohar Peled Jun 09 '15 at 06:23
  • @ZoharPeled : just as you said, more details. "initialized with the same seed" doesn't really explain why it it so. – Falanwe Jun 09 '15 at 10:15
  • @Falanwe: I've added a link to Matthew's comment. I think it's better then plagiarizing it... – Zohar Peled Jun 09 '15 at 10:34

2 Answers2

12

The reason is that you are initializing the Random object inside the method.
When you call the method in close time proximity (like inside a loop), the Random object is initialized with the same seed. (see Matthew Watson's comment to find out why.)
To prevent that you should declare and initialize the Random object as a static field, like this:

public class RandomKey
{
    static Random rng = new Random();

    public static string GetKey() 
    {
    // do your stuff...
    }
}
Community
  • 1
  • 1
Zohar Peled
  • 79,642
  • 10
  • 69
  • 121
4

You keep reinitializing the Random value. Move that out to a static field. Also you can format numbers in hex using ToString with a formatter.

Also, DateTime.Now is a bad idea. See this answer for a better way to allocate unique, timestamp values.

Community
  • 1
  • 1
Ian Mercer
  • 38,490
  • 8
  • 97
  • 133