patterncsharpMajor
Random string generation
Viewed 0 times
randomgenerationstring
Problem
I'm using this C# function to generate random coupons for a system. How can I improve it?
Business requirements are:
public static string GenerateCoupon(int length)
{
string result = string.Empty;
Random random = new Random((int)DateTime.Now.Ticks);
List characters = new List() { };
for (int i = 48; i < 58; i++)
{
characters.Add(((char)i).ToString());
}
for (int i = 65; i < 91; i++)
{
characters.Add(((char)i).ToString());
}
for (int i = 97; i < 123; i++)
{
characters.Add(((char)i).ToString());
}
for (int i = 0; i < length; i++)
{
result += characters[random.Next(0, characters.Count)];
Thread.Sleep(1);
}
return result;
}Business requirements are:
- The length of coupons are not fixed and static
- Coupons can contain A-Z, a-z and 0-9 (case sensitive alphanumeric)
- Coupons should be unique (which means that we store them as key in a table in database, and for each coupon, we check its uniqueness)
Solution
Let's look at some of the code:
You don't need to create a seed for the Random constructor from the clock, the parameterless constructor does that:
You don't need the initialiser brackets when you don't put any items in the list at that point:
Using
Why on earth are you sleeping in the middle of the loop?
Instead of creating a list of strings, just use a string literal to pick characters from:
Consider if all those characters should be included, or wheter you should omit similar looking characters like
Edit:
If you are going to call the method more than once, you should send in the random generator as a parameter:
Usage:
Example output:
Random random = new Random((int)DateTime.Now.Ticks);You don't need to create a seed for the Random constructor from the clock, the parameterless constructor does that:
Random random = new Random();List characters = new List() { };You don't need the initialiser brackets when you don't put any items in the list at that point:
List characters = new List();result += characters[random.Next(0, characters.Count)];Using
+= to concatenate strings is bad practice. Strings are not appended at the end, as strings are immutable. Code like x += y; actually end up as x = String.Concat(x, y). You should rather use a StringBuilder to build the string.Thread.Sleep(1);Why on earth are you sleeping in the middle of the loop?
Instead of creating a list of strings, just use a string literal to pick characters from:
public static string GenerateCoupon(int length) {
Random random = new Random();
string characters = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
StringBuilder result = new StringBuilder(length);
for (int i = 0; i < length; i++) {
result.Append(characters[random.Next(characters.Length)]);
}
return result.ToString();
}Consider if all those characters should be included, or wheter you should omit similar looking characters like
o, O and 0. Having the characters in a string literal makes it easy to do that.Edit:
If you are going to call the method more than once, you should send in the random generator as a parameter:
public static string GenerateCoupon(int length, Random random) {
string characters = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";
StringBuilder result = new StringBuilder(length);
for (int i = 0; i < length; i++) {
result.Append(characters[random.Next(characters.Length)]);
}
return result.ToString();
}Usage:
Random rnd = new Random();
string[] coupon = new string[10];
for (int i = 0; i < coupon.Length; i++) {
coupon[i] = GenerateCoupon(10, rnd);
}
Console.WriteLine(String.Join(Environment.NewLine,coupon));Example output:
LHUSer9dPZ
btK0S01yLb
hruw4IXINJ
hwMdRDRujt
cr4TDezvcZ
b8tVETNXNL
JrG6sfXgZF
Y7FRypnRiQ
JbfnhY3qOx
quWNakbybYCode Snippets
Random random = new Random((int)DateTime.Now.Ticks);Random random = new Random();List<string> characters = new List<string>() { };List<string> characters = new List<string>();result += characters[random.Next(0, characters.Count)];Context
StackExchange Code Review Q#5983, answer score: 33
Revisions (0)
No revisions yet.