In this case, if performance are not a MEASURED issue (read: you're not generating 1,000,000 passwords) then I'd definitely pick the most readable version.
It might even be something like this (completely and absolutely untested) which - in average - should also perform better than original implementation:
using (var rng = new RNGCryptoServiceProvider())
var buffer = new byte[length * 5];
var password = new StringBuilder(length);
while (password.Length < length)
.Select(x => (char)x)
.Take(length - password.Length)
- Decision to use
length * 5 is arbitrary (you take only 58 values from the 256 possible). For small strings there are good chances that it doesn't work well, run this few thousands times and see how many times one call to
GetBytes() is not enough.
- It performs pretty well (
GetBytes() overhead is - typically - payed only once) and you're not building lists of expensive structures (
StringBuilder is highly optimized to build strings).
- It should be slightly more fast than base64 encoding (which otherwise is a clever idea).
- It's really easy to optimize for the best case scenario adding some more code (for example when
ToArray() returns the required string you can avoid
StringBuilder() - and its initial allocation - all together).
- It scales well (parallel generation of N passwords or even generation of a huge password in parallel from M chunks).
- Given that your input is a single byte then you can use
Char.IsLetterOrDigit directly because all the other non Latin letters and digits are > 255 when UTF-16 encoded).
The very next (or previous?) step is to test this code. Can you reliably an extensively test a function which uses a random number generator? Hardly. t3schb0t covers this aspect in his answer.
Are we done? There are some security concerns you should consider, see last section.
As you did imagine
goto is the controversial part of your code. Let me address this separately because, it seems, you're prone to use it.
goto usually hurts legibility and you're also (probably) preventing some optimizations compiler might do. Depending where in the compiler pipeline the optimization is applied (near the front-end where
else differ, or near the back-end - or even after JIT - where they already generated the exact same code) a compiler is probably not optimizing
goto (because it's uncommon to be worth of the time and when you're using it then you know what you're doing). In short: if it's because of performance than 99/100 compiler can do a better job than you.
I'm not saying that there are not use-cases (there are) but hey're so rare that hat you may live without writing a
goto for most of your career (with few more chances for its little brother
goto case). Please do not generalize this to every language, in C - for example - it might be useful for clean-up code and error handling but in higher level languages we have other powerful tools.
Does it make the code shorter or more readable? In your specific case, even without changing anything else, the very same code might be rewritten as:
while (chars.Count < length)
char nextChar = (char)buffer;
What did we do? We moved some logic into a separate function, this has two advantages:
- It makes logic easier to follow because we can read the function name instead of understanding the code.
- If frees us from the need of that
goto because we can reduce those multiple
What if there wasn't
Char.IsLetterOrDigit()? You write your own and 90% you will get rid of those jumps. There are more things in the original code (beside
counter is useless, you can use an array instead of a linked list (size is known), it's preferable to avoid
var for primitive types, you're hard-coding magic constants, and...you're reducing the entropy of the generated password.
You're correctly using
RNGCryptoServiceProvider to generate the random numbers however you're not using a proper one-to-one encoding algorithm to transform those numbers into text. Simply dropping some values reduces the entropy. This is unavoidable if you want a fixed length alphanumeric string.
However if your goal is to generate a password with the desired amount of entropy then for this step you should use base62 (as elsewhere suggested base64 for performance reasons), a Diceware algorithm, or a perfect hash function (no, you can't use modulus for this because it, obviously, has multiple collisions).