Mixing
Random alphanumeric password generator with GOTOs
Clash Royale CLAN TAG#URR8PPP
up vote
3
down vote
favorite
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);
c# random
edited 33 mins ago
t3chb0t
33.1k744106
asked 48 mins ago
Sam Pearson
25219
add a comment |Â
up vote
3
down vote
favorite
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);
c# random
edited 33 mins ago
t3chb0t
33.1k744106
asked 48 mins ago
Sam Pearson
25219
2
Is there a particular reason you're usinggoto? In this example it's pretty obvious what it's doing, but I've seen some legacy code that was a crippling headache to figure out what agotowas doing when a simple function would have been as clear as glass.
– Shelby115
21 mins ago
Is usinggotoan essential motivator for this code? If not, then it probably doesn't warrant a mention in the title.
– Toby Speight
1 min ago
add a comment |Â
up vote
3
down vote
favorite
up vote
3
down vote
favorite
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);
c# random
edited 33 mins ago
t3chb0t
33.1k744106
asked 48 mins ago
Sam Pearson
25219
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);
c# random
c# random
edited 33 mins ago
t3chb0t
33.1k744106
asked 48 mins ago
Sam Pearson
25219
edited 33 mins ago
t3chb0t
33.1k744106
asked 48 mins ago
Sam Pearson
25219
edited 33 mins ago
t3chb0t
33.1k744106
edited 33 mins ago
t3chb0t
33.1k744106
edited 33 mins ago
t3chb0t
33.1k744106
33.1k744106
asked 48 mins ago
Sam Pearson
25219
asked 48 mins ago
Sam Pearson
25219
asked 48 mins ago
Sam Pearson
25219
25219
2
Is there a particular reason you're usinggoto? In this example it's pretty obvious what it's doing, but I've seen some legacy code that was a crippling headache to figure out what agotowas doing when a simple function would have been as clear as glass.
– Shelby115
21 mins ago
Is usinggotoan essential motivator for this code? If not, then it probably doesn't warrant a mention in the title.
– Toby Speight
1 min ago
add a comment |Â
2
Is there a particular reason you're usinggoto? In this example it's pretty obvious what it's doing, but I've seen some legacy code that was a crippling headache to figure out what agotowas doing when a simple function would have been as clear as glass.
– Shelby115
21 mins ago
Is usinggotoan essential motivator for this code? If not, then it probably doesn't warrant a mention in the title.
– Toby Speight
1 min ago
2
2
Is there a particular reason you're using
goto? In this example it's pretty obvious what it's doing, but I've seen some legacy code that was a crippling headache to figure out what a goto was doing when a simple function would have been as clear as glass.– Shelby115
21 mins ago
Is there a particular reason you're using
goto? In this example it's pretty obvious what it's doing, but I've seen some legacy code that was a crippling headache to figure out what a goto was doing when a simple function would have been as clear as glass.– Shelby115
21 mins ago
Is using
goto an essential motivator for this code? If not, then it probably doesn't warrant a mention in the title.– Toby Speight
1 min ago
Is using
goto an essential motivator for this code? If not, then it probably doesn't warrant a mention in the title.– Toby Speight
1 min ago
add a comment |Â
3 Answers
3
active
oldest
votes
up vote
3
down vote
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.
I just noticed as well a bug in your existing code. If a character is chosen that isn't allowed it is just skipped instead of choosing another character.
answered 19 mins ago
tinstaafl
5,842625
I think it wouldn't be wise to bangotoentirely... expression trees use them so it's good to be familiar with them ;-]
– t3chb0t
14 mins ago
@t3chb0t - It seems to me that sinceGotois an overloaded extension in theExpressionclass there shouldn't be a need for them in the general language.
– tinstaafl
6 mins ago
add a comment |Â
up vote
2
down vote
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)
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).
answered 15 mins ago
Shelby115
1,416516
1
You might be doing more work than the usual short-circuit equivalent, by storing comparison results in those (nicely named!) variables before testing. Unless your compiler optimises that away, of course.
– Toby Speight
3 mins ago
add a comment |Â
up vote
2
down vote
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())
while (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.
answered 10 mins ago
200_success
126k14148409
Linked lists involve a lot of overhead for each node. - do they? I many cases they are pretty damn fast.
– t3chb0t
7 mins ago
1
@t3chb0t "Another disadvantage of linked lists is the extra storage needed for references, which often makes them impractical for lists of small data items such as characters or boolean values, because the storage overhead for the links may exceed by a factor of two or more the size of the data. … It can also be slow, and with a naïve allocator, wasteful, to allocate memory separately for each new element, a problem generally solved using memory pools." Definitely much much worse than acharfor this application.
– 200_success
4 mins ago
add a comment |Â
3 Answers
3
active
oldest
votes
3 Answers
3
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
3
down vote
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.
I just noticed as well a bug in your existing code. If a character is chosen that isn't allowed it is just skipped instead of choosing another character.
answered 19 mins ago
tinstaafl
5,842625
I think it wouldn't be wise to bangotoentirely... expression trees use them so it's good to be familiar with them ;-]
– t3chb0t
14 mins ago
@t3chb0t - It seems to me that sinceGotois an overloaded extension in theExpressionclass there shouldn't be a need for them in the general language.
– tinstaafl
6 mins ago
add a comment |Â
up vote
3
down vote
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.
I just noticed as well a bug in your existing code. If a character is chosen that isn't allowed it is just skipped instead of choosing another character.
answered 19 mins ago
tinstaafl
5,842625
I think it wouldn't be wise to bangotoentirely... expression trees use them so it's good to be familiar with them ;-]
– t3chb0t
14 mins ago
@t3chb0t - It seems to me that sinceGotois an overloaded extension in theExpressionclass there shouldn't be a need for them in the general language.
– tinstaafl
6 mins ago
add a comment |Â
up vote
3
down vote
up vote
3
down vote
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.
I just noticed as well a bug in your existing code. If a character is chosen that isn't allowed it is just skipped instead of choosing another character.
answered 19 mins ago
tinstaafl
5,842625
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.
I just noticed as well a bug in your existing code. If a character is chosen that isn't allowed it is just skipped instead of choosing another character.
answered 19 mins ago
tinstaafl
5,842625
answered 19 mins ago
tinstaafl
5,842625
answered 19 mins ago
tinstaafl
5,842625
answered 19 mins ago
tinstaafl
5,842625
5,842625
I think it wouldn't be wise to bangotoentirely... expression trees use them so it's good to be familiar with them ;-]
– t3chb0t
14 mins ago
@t3chb0t - It seems to me that sinceGotois an overloaded extension in theExpressionclass there shouldn't be a need for them in the general language.
– tinstaafl
6 mins ago
add a comment |Â
I think it wouldn't be wise to bangotoentirely... expression trees use them so it's good to be familiar with them ;-]
– t3chb0t
14 mins ago
@t3chb0t - It seems to me that sinceGotois an overloaded extension in theExpressionclass there shouldn't be a need for them in the general language.
– tinstaafl
6 mins ago
I think it wouldn't be wise to ban
goto entirely... expression trees use them so it's good to be familiar with them ;-]– t3chb0t
14 mins ago
I think it wouldn't be wise to ban
goto entirely... expression trees use them so it's good to be familiar with them ;-]– t3chb0t
14 mins ago
@t3chb0t - It seems to me that since
Goto is an overloaded extension in the Expression class there shouldn't be a need for them in the general language.– tinstaafl
6 mins ago
@t3chb0t - It seems to me that since
Goto is an overloaded extension in the Expression class there shouldn't be a need for them in the general language.– tinstaafl
6 mins ago
add a comment |Â
up vote
2
down vote
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)
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).
answered 15 mins ago
Shelby115
1,416516
1
You might be doing more work than the usual short-circuit equivalent, by storing comparison results in those (nicely named!) variables before testing. Unless your compiler optimises that away, of course.
– Toby Speight
3 mins ago
add a comment |Â
up vote
2
down vote
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)
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).
answered 15 mins ago
Shelby115
1,416516
1
You might be doing more work than the usual short-circuit equivalent, by storing comparison results in those (nicely named!) variables before testing. Unless your compiler optimises that away, of course.
– Toby Speight
3 mins ago
add a comment |Â
up vote
2
down vote
up vote
2
down vote
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)
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).
answered 15 mins ago
Shelby115
1,416516
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)
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).
answered 15 mins ago
Shelby115
1,416516
answered 15 mins ago
Shelby115
1,416516
answered 15 mins ago
Shelby115
1,416516
answered 15 mins ago
Shelby115
1,416516
1,416516
1
You might be doing more work than the usual short-circuit equivalent, by storing comparison results in those (nicely named!) variables before testing. Unless your compiler optimises that away, of course.
– Toby Speight
3 mins ago
add a comment |Â
1
You might be doing more work than the usual short-circuit equivalent, by storing comparison results in those (nicely named!) variables before testing. Unless your compiler optimises that away, of course.
– Toby Speight
3 mins ago
1
1
You might be doing more work than the usual short-circuit equivalent, by storing comparison results in those (nicely named!) variables before testing. Unless your compiler optimises that away, of course.
– Toby Speight
3 mins ago
You might be doing more work than the usual short-circuit equivalent, by storing comparison results in those (nicely named!) variables before testing. Unless your compiler optimises that away, of course.
– Toby Speight
3 mins ago
add a comment |Â
up vote
2
down vote
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())
while (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.
answered 10 mins ago
200_success
126k14148409
Linked lists involve a lot of overhead for each node. - do they? I many cases they are pretty damn fast.
– t3chb0t
7 mins ago
1
@t3chb0t "Another disadvantage of linked lists is the extra storage needed for references, which often makes them impractical for lists of small data items such as characters or boolean values, because the storage overhead for the links may exceed by a factor of two or more the size of the data. … It can also be slow, and with a naïve allocator, wasteful, to allocate memory separately for each new element, a problem generally solved using memory pools." Definitely much much worse than acharfor this application.
– 200_success
4 mins ago
add a comment |Â
up vote
2
down vote
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())
while (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.
answered 10 mins ago
200_success
126k14148409
Linked lists involve a lot of overhead for each node. - do they? I many cases they are pretty damn fast.
– t3chb0t
7 mins ago
1
@t3chb0t "Another disadvantage of linked lists is the extra storage needed for references, which often makes them impractical for lists of small data items such as characters or boolean values, because the storage overhead for the links may exceed by a factor of two or more the size of the data. … It can also be slow, and with a naïve allocator, wasteful, to allocate memory separately for each new element, a problem generally solved using memory pools." Definitely much much worse than acharfor this application.
– 200_success
4 mins ago
add a comment |Â
up vote
2
down vote
up vote
2
down vote
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())
while (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.
answered 10 mins ago
200_success
126k14148409
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())
while (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.
answered 10 mins ago
200_success
126k14148409
answered 10 mins ago
200_success
126k14148409
answered 10 mins ago
200_success
126k14148409
answered 10 mins ago
200_success
126k14148409
126k14148409
Linked lists involve a lot of overhead for each node. - do they? I many cases they are pretty damn fast.
– t3chb0t
7 mins ago
1
@t3chb0t "Another disadvantage of linked lists is the extra storage needed for references, which often makes them impractical for lists of small data items such as characters or boolean values, because the storage overhead for the links may exceed by a factor of two or more the size of the data. … It can also be slow, and with a naïve allocator, wasteful, to allocate memory separately for each new element, a problem generally solved using memory pools." Definitely much much worse than acharfor this application.
– 200_success
4 mins ago
add a comment |Â
Linked lists involve a lot of overhead for each node. - do they? I many cases they are pretty damn fast.
– t3chb0t
7 mins ago
1
@t3chb0t "Another disadvantage of linked lists is the extra storage needed for references, which often makes them impractical for lists of small data items such as characters or boolean values, because the storage overhead for the links may exceed by a factor of two or more the size of the data. … It can also be slow, and with a naïve allocator, wasteful, to allocate memory separately for each new element, a problem generally solved using memory pools." Definitely much much worse than acharfor this application.
– 200_success
4 mins ago
Linked lists involve a lot of overhead for each node. - do they? I many cases they are pretty damn fast.
– t3chb0t
7 mins ago
Linked lists involve a lot of overhead for each node. - do they? I many cases they are pretty damn fast.
– t3chb0t
7 mins ago
1
1
@t3chb0t "Another disadvantage of linked lists is the extra storage needed for references, which often makes them impractical for lists of small data items such as characters or boolean values, because the storage overhead for the links may exceed by a factor of two or more the size of the data. … It can also be slow, and with a naïve allocator, wasteful, to allocate memory separately for each new element, a problem generally solved using memory pools." Definitely much much worse than a
char for this application.– 200_success
4 mins ago
@t3chb0t "Another disadvantage of linked lists is the extra storage needed for references, which often makes them impractical for lists of small data items such as characters or boolean values, because the storage overhead for the links may exceed by a factor of two or more the size of the data. … It can also be slow, and with a naïve allocator, wasteful, to allocate memory separately for each new element, a problem generally solved using memory pools." Definitely much much worse than a
char for this application.– 200_success
4 mins ago
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f207171%2frandom-alphanumeric-password-generator-with-gotos%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
2
Is there a particular reason you're using
goto? In this example it's pretty obvious what it's doing, but I've seen some legacy code that was a crippling headache to figure out what agotowas doing when a simple function would have been as clear as glass.– Shelby115
21 mins ago
Is using
gotoan essential motivator for this code? If not, then it probably doesn't warrant a mention in the title.– Toby Speight
1 min ago