-2
\$\begingroup\$

The method below is returning a string of random characters using RNGCryptoServiceProvider. The return string result is built by picking characters from the string chars by applying % chars.length on the byte values (0-255) in the array of bytes returned by GetBytes(). This means that some characters will be favoured over others, depending on the length of chars.

Can this method be re-written so that all the characters in chars have an equal chance of being picked?

/// <summary>
/// Returns a string of cryptographically sound random characters
/// </summary>
/// <param name="type">Accepted parameter variables are HEX (0-F), hex (0-f),
/// DEC/dec/NUM/num (0-9), ALPHA (A-Z), alpha (a-z), ALPHANUM (A-Z and 0-9),
/// alphanum (a-z and 0-9) and FULL/full (A-Z, a-z and 0-9)</param>
/// <param name="length">The length of the output string</param>
/// <returns>String of cryptographically sound random characters</returns>
public static string Serial(string type, int length)
{
    if (length < 1) return "";
    string chars;
    switch (type)
    {
        case "HEX":
            chars = "0123456789ABCDEF";
            break;
        case "hex":
            chars = "0123456789abcdef";
            break;
        case "DEC":
            chars = "0123456789";
            break;
        case "dec":
            chars = "0123456789";
            break;
        case "NUM":
            chars = "0123456789";
            break;
        case "num":
            chars = "0123456789";
            break;
        case "ALPHA":
            chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
            break;
        case "alpha":
            chars = "abcdefghijklmnopqrstuvwxyz";
            break;
        case "ALPHANUM":
            chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
            break;
        case "alphanum":
            chars = "abcdefghijklmnopqrstuvwxyz0123456789";
            break;
        case "FULL":
            chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
            break;
        case "full":
            chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
            break;
        default:
            chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
            break;
    }
    byte[] data = new byte[length];
    using (RNGCryptoServiceProvider crypto = new RNGCryptoServiceProvider())
    {
        crypto.GetBytes(data);
    }
    StringBuilder result = new StringBuilder(length);
    foreach (byte b in data)
    {
        result.Append(chars[b % (chars.Length)]);
    }
    return result.ToString();
}
\$\endgroup\$

closed as off-topic by Toby Speight, Stephen Rauch, yuri, 200_success, t3chb0t Aug 9 '18 at 17:30

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – Toby Speight, Stephen Rauch, yuri, 200_success, t3chb0t
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 4
    \$\begingroup\$ I would say that this is off-topic, as the code has a known bug. Possibly Stack Overflow is a better place for this. \$\endgroup\$ – Alejandro Aug 9 '18 at 14:06
2
\$\begingroup\$

The only method that I'm aware of to avoid modulo bias is to just exclude some bytes so that the number of possible values is a multiple of the array's length, and all the numbers are consecutive. Something like this should work if your strings are not more than 256 characters long.

random_index(length):
    m = 256 % length
    n = next_byte()
    while n < m:
        n = next_byte()
    return n % length

However, there are two problems with this approach. Firstly, this will need an unpredictable number of bytes, so you need to be able to get more on-demand. Secondly, I'm no expert in C#, but this code seems way too low-level. Any decent library should provide some kind of choose function you can use to simply pick a character from an array.

\$\endgroup\$
2
\$\begingroup\$
  1. IMO the type parameter calls for an enum:

    public enum SerialType
    {
      HEX = 1,
      hex = 2,
      ...
    }
    
  2. The two last cases and the default can be combined in one entry:

        switch (type)
        {
          ...
          case "FULL":
          case "full":
          default:
            chars = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
            break;
        }
    

    (and you can do the same with DEC, dec, NUM and num - so why not only have one of them?)

  3. You can build the result in this way:

    string.Join("", data.Select(b => chars[b % chars.Length]));
    
\$\endgroup\$
  • 1
    \$\begingroup\$ Converting a byte to a double doesn't change that there are only 256 possible values. If that's not evenly divisible by the number of possible characters then there's going to be a bias either way: some characters will have more values 'mapping' to them than others. \$\endgroup\$ – Pieter Witvoet Aug 9 '18 at 12:37
  • \$\begingroup\$ @PieterWitvoet: You're right. I'm a little naive here. \$\endgroup\$ – Henrik Hansen Aug 9 '18 at 13:38

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