11
\$\begingroup\$

Below is my code for generating a random string of random length. I am using it in integration tests. Currently I have almost 2000 tests running each time a commit is made to master and a few of those tests are using this class.

I would like to make sure the code is optimized so that the tests complete as quickly as possible.

public class Strings
{
    private static readonly Random Random = new Random();

    private static int RandomChar => Random.Next(char.MinValue, char.MaxValue);

    public static string GenerateRandomStringOfSetLength(int length)
    {
        return GenerateRandomString(length);
    }

    private static string GenerateRandomString(int length)
    {
        var stringBuilder = new StringBuilder();

        while (stringBuilder.Length - 1 <= length)
        {
            var character = Convert.ToChar(RandomChar);
            if (!char.IsControl(character))
            {
                stringBuilder.Append(character);
            }
        }

        return stringBuilder.ToString();
    }
}
\$\endgroup\$
  • \$\begingroup\$ Why do you need random strings ? \$\endgroup\$ – Denis Jan 13 '17 at 14:52
  • 2
    \$\begingroup\$ Retorical question: How valid are these tests; any given test? How can random input on every test run be covering the intended conditions? Unless test output results are being compared. But that sounds like a test dependency that should be avoided. If this is unit testing, we don't want to use a database precisely because of its variable/uncontrolled state. \$\endgroup\$ – radarbob Jan 13 '17 at 14:55
  • \$\begingroup\$ The intention of these random strings are purely to add some form of realism to the integration tests I've built to ensure that characters I would normally not think of from my English speak perspective won't unintentionally break my code. Maybe this can be seen as showing a lack of knowledge in my chosen platform, however I approach it from the point of view of completeness in my tests. Also, These random strings are not be not used in a security scenario as generating "random" passwords, but purely for testing purposes. \$\endgroup\$ – Ebbs Jan 13 '17 at 19:37
  • \$\begingroup\$ @radarbob I forgot to add that I am not using or hitting a real DB in my tests either. I am rather generating the tables I need in memory from my EF models. This random string class is part of the routine of seeding those tables. \$\endgroup\$ – Ebbs Jan 13 '17 at 19:56
  • 2
    \$\begingroup\$ Randomizing tests is a bad idea. Suppose a test fails one time in a million; how are you going to be able to reproduce it to find the problem? Generate your random data once and hard code it into the tests. \$\endgroup\$ – Eric Lippert Jan 13 '17 at 21:35
16
\$\begingroup\$

Just two remarks:

  • RandomChar should be a method because it returns a different result each time - this is just a convention that we usually follow in C#

In general, methods represent actions and properties represent data. Properties are meant to be used like fields, meaning that properties should not be computationally complex or produce side effects.

from Choosing Between Properties and Methods

so

Do use a property, rather than a method, if the value of the property is stored in the process memory and the property would just provide access to the value.

but

Do use a method, rather than a property, in the following situations.

amonong many others:

  • The operation returns a different result each time it is called, even if the parameters do not change. For example, the NewGuid method returns a different value each time it is called.

  • You can cast the result to char and don't need the Convert.ToChar

_

private static char RandomChar() => (char)Random.Next(char.MinValue, char.MaxValue);

You can also improve the performance by precalculating the array with chars:

private static readonly char[] Chars = 
    Enumerable
    .Range(char.MinValue, char.MaxValue)
    .Select(x => (char)x)
    .Where(c => !char.IsControl(c))
    .ToArray();

the RandomChar method would take the values from this array:

private static char RandomChar() => Chars[Random.Next(0, Chars.Length)];

so building the string can be a simple loop:

for (int i = 0; i < length; i++)
{
    stringBuilder.Append(RandomChar());
}

Without the StringBuilder this seems to be faster in tests by just ~8ms for 100.000 loops and a string lenght of 1.000

var chars = new char[length];
for (int i = 0; i < length; i++)
{
    chars[i] = RandomChar();
}
return new string(chars);

StringBuilder.ToString processes the data in order to build the string. It's quite fast but in this particular situation the minimal overhead is noticable. Generally the StringBuilder is the fastet way to build strings so using it is definitely the right choice.


Changing the Random.Next(0, max) to Random.Next(max) improves the performance by another 10ms for the same tests.

\$\endgroup\$
  • 1
    \$\begingroup\$ I am interested about the convention where you put non-deterministic things in their own method. Does that have a name or is somewhere I can read about it? \$\endgroup\$ – Evorlor Jan 14 '17 at 0:57
  • 1
    \$\begingroup\$ To echo what @Evorlor said, I am also interested in what you stated about using a method as apposed to a property. From my point of view the code looks cleaner using a property. Perhaps a link? Thanks for your help and a clear and concise answer. I previously also tried pre-calculating a char array, going a slightly different route and ending up with the class taking 2 - 3s to execute. Do you have any specific technical insights as to why the pure char array method seems to be faster compared to using StringBuilder(length)? \$\endgroup\$ – Ebbs Jan 14 '17 at 9:30
  • 1
    \$\begingroup\$ @Evorlor I've added more infromation about it \$\endgroup\$ – t3chb0t Jan 14 '17 at 9:37
  • 2
    \$\begingroup\$ @MrThursday see the updated answer, I've added more information \$\endgroup\$ – t3chb0t Jan 14 '17 at 9:37
  • 2
    \$\begingroup\$ @t3chb0t thanks for the great answers and added info. If I could upvote your answer more, I would. o7 \$\endgroup\$ – Ebbs Jan 14 '17 at 11:22
9
\$\begingroup\$

One small improvement is since you know the size of the string when you create the StringBuilder tell it how much it will hold.

 var stringBuilder = new StringBuilder(length);

This will prevent it from having to allocate more memory in the process if the string is long.

\$\endgroup\$
4
\$\begingroup\$

LINQ solution:

public string GetRandomString(int length)
{
    return String.Concat(RandomSequence().Where(x => !char.IsControl(x)).Take(length));
}

private IEnumerable<char> RandomSequence()
{
    while(true)
    {
        yield return (char)Random.Next(char.MinValue, char.MaxValue);
    }
}

Pretty short, but it is most likely the slowest option.

\$\endgroup\$
  • 1
    \$\begingroup\$ Nice and clean indeed, but you're right about the performance (just tested it). \$\endgroup\$ – t3chb0t Jan 13 '17 at 15:38
3
\$\begingroup\$

Random is pseudo random, as stated in the docs:

Represents a pseudo-random number generator, which is a device that produces a sequence of numbers that meet certain statistical requirements for randomness.

This is good for modelling, and games. However not for security. And so I'd recommend that you instead use RNGCryptoServiceProvider. This has a method, GetBytes(byte[] data), that looks exactly like what you want. Using this you can build an ASCII string, rather than a Unicode string, and will have the same level of randomness per bit/byte. But not per char, so your strings will need to be twice as long, as you're generating numbers in the range 0 to 65535, where the above is 0 to 256. Finally you can change the byte array to a string using Encoding.ASCII.GetString.

And so you can get:

private static string GenerateRandomString(int size)
{
    var b = new byte[size];
    new RNGCryptoServiceProvider().GetBytes(b);
    return Encoding.ASCII.GetString(b);
}

If you do need to have the program produce chars that are 16bits each, then you can chunk the above bytes array into chunks of two, shift the first by 8 bits, first << 8, and then add them.

\$\endgroup\$
  • \$\begingroup\$ Good point regarding security. I was actually thinking about this when I wrote the class as illustrated, however I do not intend to used it in such a scenario. I purely want a very good performing random string generator which goes beyond the normal alpha numeric characters we use in our every day English world. \$\endgroup\$ – Ebbs Jan 13 '17 at 19:47

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.