mjhjmtu

I was inspired after this thread on Stack Overflow to create a random 8 character alphanumeric password generator.

Sadly, it is closed, so I cannot provide an answer there. Anyway, here is the code. Let me know if there are any bugs/bias going on.

using System.Collections.Generic;
using System.Security.Cryptography;

public class AlphanumericGen

 public virtual string Generate(byte length)
 
 ICollection<char> chars = new LinkedList<char>();
 var buffer = new byte[1];
 short counter = 0;
 using (var rng = new RNGCryptoServiceProvider())
 
 while (counter < length)
 
 rng.GetBytes(buffer);
 var nextChar = (char)buffer[0];

 if (nextChar >= '0' && nextChar <= '9')
 
 goto addChar;
 

 if (nextChar >= 'A' && nextChar <= 'Z')
 
 goto addChar;
 

 if (nextChar >= 'a' && nextChar <= 'z')
 
 goto addChar;
 

 continue;

 addChar:
 chars.Add(nextChar);
 ++counter;
 
 

 return string.Concat(chars);
 

As a general rule of thumb, any time you feel the need to use goto, take a couple of aspirin and lay down until the feeling passes. They probably should have been deprecated decades ago.

In this particular case, using a string of allowed characters and randomly picking an index in that string would do away with your goto's.

I think too a StringBuilder would do a lot better for storing the characters that a LinkedList.

Data structure

Why are you using a LinkedList<char>? Linked lists involve a lot of overhead for each node. You know exactly how long the result should be, so why not write to a char array?

Actually, you may want to consider returning the result as a char rather than as a string. A security-sensitive program may wish to wipe the contents of the secret by overwriting it with NUL bytes, rather than waiting for the garbage collector to clean up the string.

Flow control

goto should be avoided, and its use is not justified here.

The loop is a bit clumsy, referring to counter all over the place.

char chars = new char[length];
using (var rng = new RNGCryptoServiceProvider())

 for (int counter = 0; counter < length; )
 
 return new string(chars);

Algorithm

The algorithm is rather inefficient:

• It reads a byte at a time from the random number generator. .GetBytes() is thread safe, so each call should incur some synchronization overhead.


• It throws away most of the bytes it reads, keeping each byte with a probability of $62 over 256$.

A smarter approach would be to fetch a bit more than $3 over 4 mathrmlength$ random bytes, base64-encode it, and discard just the resulting + and / characters.

Readability & GoTo

If you want to do the same action for 3 different if-statements make them one if-statement or make them a function. goto causes massive headaches when it's been months since you've touched the code and you come back to it not remembering a thing. Not to mention || is short circuit so you're not really losing anything on performance.

rng.GetBytes(buffer);
var nextChar = (char)buffer[0];

var isNumber = nextChar >= '0' && nextChar <= '9';
var isUppercaseLetter = nextChar >= 'A' && nextChar <= 'Z';
var isLowercaseLetter = nextChar >= 'a' && nextChar <= 'z';

if (isNumber || isUppercaseLetter || isLowercaseLetter)

 chars.Add(nextChar);
 ++counter;

Now with the conditions named even an idiot like me can come in here and see that you're checking if the character is alpha-numeric. If you wanted to make it even more obvious you could go one step further and make it a function.

public bool IsAlphaNumericCharacter(char c)

 var isNumber = c >= '0' && c <= '9';
 var isUppercaseLetter = c >= 'A' && c <= 'Z';
 var isLowercaseLetter = c >= 'a' && c <= 'z';

 return isNumber
 

Then your loop becomes shorter and clearer.

while (counter < length)

 rng.GetBytes(buffer);
 var nextChar = (char)buffer[0];

 if (IsAlphaNumericCharacter(nextChar))
 
 chars.Add(nextChar);
 ++counter;
 

Bytes

Is there a particular reason you're using byte for length instead of int? Also, was there a reason for var buffer = new byte[1]; being an array instead of just var buffer = new byte();

If the answer to both of those questions is no, then you could have a character array of size length instead of a LinkedList<char>.

StringBuilder

StringBuilder could be beneficial, but if you're at a low amount of characters (like your link suggests) then it's probably not going to make a difference performance-wise (always best to test yourself instead of listening to a stranger on the internet though :P).