My static C# function is playing games with me… totaly weird!

Question

So, I've written a small and, from what I initially thought, easy method in C#. This static method is meant to be used as a simple password suggestion generator, and the code looks like this:

public static string CreateRandomPassword(int outputLength, string source = "")
{
  var output = string.Empty;

  for (var i = 0; i < outputLength; i++)
  {
     var randomObj = new Random();
     output += source.Substring(randomObj.Next(source.Length), 1);
  }

  return output;
}

I call this function like this:

var randomPassword = StringHelper.CreateRandomPassword(5, "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890");

Now, this method almost always return random strings like "AAAAAA", "BBBBBB", "888888" etc.. , where I thought it should return strings like "A8JK2A", "82mOK7" etc.

However, and here is the wierd part; If I place a breakpoint, and step through this iteration line by line, I get the correct type of password in return. In 100% of the other cases, when Im not debugging, it gives me crap like "AAAAAA", "666666", etc..

How is this possible? Any suggestion is greatly appreciated! :-)

BTW, my system: Visual Studio 2010, C# 4.0, ASP.NET MVC 3 RTM project w/ ASP.NET Development Server. Haven't tested this code in any other environments.

Answer 1

Move the declaration for the randomObj outside the loop. When you're debugging it, it creates it with a new seed each time, because there's enough time difference for the seed to be different. But when you're not debugging, the seed time is basically the same for each iteration of the loop, so it's giving you the same start value each time.

And a minor nit -- it's a good habit to use a StringBuilder rather than a string for this sort of thing, so you don't have to re-initialize the memory space every time you append a character to the string.

In other words, like this:

public static string CreateRandomPassword(int outputLength, string source = "")
{
  var output = new StringBuilder();
  var randomObj = new Random();
  for (var i = 0; i < outputLength; i++)
  {
     output.Append(source.Substring(randomObj.Next(source.Length), 1));
  }
  return output.ToString();
}

Answer 2

The behavior you're seeing is because Random is time-based, and when you're not debugging it flies through all 5 iterations at the same moment (more or less). So you're asking for the first random number off the same seed. When you're debugging, it takes long enough to get a new seed each time.

Move the declaration of Random outside the loop:

var randomObj = new Random();
for (var i = 0; i < outputLength; i++)
{
    output += source.Substring(randomObj.Next(source.Length), 1);
}

Now you're moving forward 5 steps away from a Random seed instead of moving 1 step away from the same Random seed 5 times.

Answer 3

You are instantiating a new instance of Random() on each iteration through the loop with a new time-dependent seed. Given the granularity of the system clock and the speed of modern CPUs, this pretty much guarantees that you restart the pseudo-random sequence over and over with the same seed.

Try something like the following, though if you're single-threaded, you can safely omit the lock():

private static Random randomBits = new Random() ;
public static string CreateRandomPassword(int outputLength, string source = "")
{
  StringBuilder sb = new StringBuilder(outputLength) ;
  lock ( randomBits )
  {
    while ( sb.Length < outputLength )
    {
      sb.Append( randomBits.Next( source.Length) , 1 ) ;
    }
  }
  return sb.ToString() ;
}

You only instantiate the RNG once. Every draws bits from the same RNG, so it should behave much more like a source of entropy. If you need repeatability for testing, use the Random constructor overload that lets you supply the seed. Same seed == same pseudo-random sequence.