Problem
I’m using this C# function to generate random coupons for a system. How can I improve it?
public static string GenerateCoupon(int length)
{
string result = string.Empty;
Random random = new Random((int)DateTime.Now.Ticks);
List<string> characters = new List<string>() { };
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:
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<string> characters = new List<string>() { };
You don’t need the initialiser brackets when you don’t put any items in the list at that point:
List<string> characters = new List<string>();
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
quWNakbybY
You should not be using Random to generate coupons. Your coupons will be somewhat predictable: if someone can see a few coupons (especially a few consecutive coupons), they will be able to reconstruct the seed and generate all the coupons. Random is ok for most numeric simulations and for some games, it’s not good at all when you need to generate unpredictable values. Your coupons act like passwords; you need cryptographic-quality randomness. Fortunately, there is a crypto-quality random generator in the C# library: System.Security.Cryptography.RNGCryptoServiceProvider.
This RNG returns bytes. There are 256 possible values in a byte. Your coupons can only use one of 62 characters, so you need to reject bytes that do not map to ASCII letters or digits.
Also, you should use StringBuilder when building a string chunk by chunk. Resolve it into a string when you’ve finished building it.
var couponLength = 32;
StringBuilder coupon = new StringBuilder(couponLength);
RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider();
byte[] rnd = new byte[1];
int n = 0;
while (n < couponLength) {
rng.GetBytes(rnd);
char c = (char)rnd[0];
if ((Char.IsDigit(c) || Char.IsLetter(c)) && rnd[0] < 127) {
++n;
coupon.Append(c);
}
}
return coupon.ToString();
You can make the generation about 4 times faster by rejecting fewer values. Instead of accepting only the 62 values that map to the characters you want, divide by 4 to get one of 64 equiprobable values, and accept 62 of these (mapping them to the right characters) and reject 2.
while (n < couponLength) {
rng.GetBytes(rnd);
rnd[0] %= 64;
if (rnd[0] < 62) {
++n;
coupon.Append((char)((rnd[0] <= 9 ? '0' : rnd[0] <= 35 ? 'A' - 10 : 'a' - 36) + rnd[0]);
}
}
Some general idea, I hope all work in C# too. Feel free to edit the answer if it is not a proper C# syntax.
1, Change the type of the characters list to char and change the loop variable to char too. This way you don’t have to cast and the for loops are easier to read:
List<char> characters = new List<char>() { };
for (char c = '0'; i <= '9'; c++) {
characters.Add(c);
}
...
for (int i = 0; i < length; i++){
result += characters[random.Next(0, characters.Count)];
}
2, Is there any reason of the Thread.Sleep(1);. It looks unnecessary.
3, I’d remove 0, O, o and l, 1 from the list. It’s easy to mix up them.
4, I’d pull out an AllowedCharacters method:
public static List<char> AllowedCharacters() {
List<char> characters = new List<char>() { };
for (char c = '0'; i <= '9'; c++) {
characters.Add(c);
}
...
characters.Remove('O');
characters.Remove('0');
...
return characters;
}
public static string GenerateCoupon(int length)
{
string result = string.Empty;
Random random = new Random((int)DateTime.Now.Ticks);
List<string> characters = AllowedCharacters();
for (int i = 0; i < length; i++) {
result += characters[random.Next(0, characters.Count)];
}
return result;
}
here is the code I will implement. Its alot faster and simpler
public static string GenerateCoupon(int length)
{
return Guid.NewGuid().ToString().Replace("-", string.Empty).Substring(0, 10);
}
Using the guild gaurantees uniqueness so your coupon codes never overlap.
If it’s acceptable to have a slightly longer string, you could use the ShortGuid class. This takes a Guid and makes it slightly more readable than the 32 byte format you’re use to ({xxx-xxx-xxx…}).
The author’s example is:
c9a646d3-9c61-4cb7-bfcd-ee2522c8f633}
shortened to:
00amyWGct0y_ze4lIsj2Mw
That may be slightly too long for a coupon code. One other suggestion is a using a pronounceable password generator, the link is something I converted from Java source a while back. Some uppercased examples:
COLINITABO
OWNSATLEDG
GORGIRRUGO
NOCKAYWIVI
FAWGILLIOL