26
\$\begingroup\$

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:

  1. The length of coupons are not fixed and static
  2. Coupons can contain A-Z, a-z and 0-9 (case sensitive alphanumeric)
  3. 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)
\$\endgroup\$
  • \$\begingroup\$ Here's a Stack Overflow answer of mine regarding precisely this. \$\endgroup\$ – Jesse C. Slicer Nov 12 '11 at 14:28
  • \$\begingroup\$ I still think that my own solution is much more readable than yours, and beyond that, I think the solution of palacsint is the most readable one, which implements SRP also (two methods, one for getting the allowed characters even from an external configuration source, and one for generating random strings out of that list). :) \$\endgroup\$ – Saeed Neamati Nov 12 '11 at 14:33
  • \$\begingroup\$ To each their own. \$\endgroup\$ – Jesse C. Slicer Nov 12 '11 at 17:18
  • \$\begingroup\$ I think in general you probably ought to avoid using both upper and lower case letters. Especially if you're including numbers... What font are you outputting these in - how do the characters I (upper-case I), l (lower-case L) and 1 (the number 1) appear? \$\endgroup\$ – Clockwork-Muse Nov 16 '11 at 23:47
  • \$\begingroup\$ @X-Zero using both upper & lower increases the availably data space (number of combinations) by a huge factor. Why wouldn't you use these? If they are to be entered by humans, choose a good font that makes the difference between 1, I & l apparent (i.e. don't let the marketing team stuff it up) \$\endgroup\$ – Kirk Broadhurst Nov 17 '11 at 23:43

10 Answers 10

32
\$\begingroup\$

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
\$\endgroup\$
  • 1
    \$\begingroup\$ I sleep 1 millisecond, because, generated coupons would be the same. Try it. Microsoft says that the Random algorithm is based on time, and when you simply create many random numbers in one CPU clock tick, then you get the same numbers for each random generation. \$\endgroup\$ – Saeed Neamati Nov 13 '11 at 13:03
  • 11
    \$\begingroup\$ @SaeedNeamati: It's only the initial seed that is based on the clock, the following random numbers are based on the previous, not on the clock. So, sleeping between each use of Next has no effect at all. If you are going to call GenerateCoupon more than once, you should create one random generator outside the method, and pass it in as a parameter. \$\endgroup\$ – Guffa Nov 13 '11 at 13:17
  • \$\begingroup\$ +1 for your good answer, :). But give it a try yourself. Create a simple loops (something like 500 times), and print the randomly generated numbers to the screen. See the result. Even if you pass it to the generation method as a parameter. I'd love to be wrong, since it means that I'll gain 1 millisecond of performance boost, per each generated coupon. \$\endgroup\$ – Saeed Neamati Nov 13 '11 at 14:24
  • 1
    \$\begingroup\$ @SaeedNeamati: Then you are doing something wrong... I added an example above. \$\endgroup\$ – Guffa Nov 13 '11 at 16:47
  • 2
    \$\begingroup\$ @EdmundSchweppe: Yes, that is why I wrote above that he should create one random generator outside the method, and pass it in as a parameter. \$\endgroup\$ – Guffa Nov 14 '11 at 21:44
22
\$\begingroup\$

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.

StringBuilder coupon = new StringBuilder();
RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider();
byte[] rnd = new byte[1];
int n = 0;
while (n < coupon.Length) {
    rng.GetBytes(rnd);
    char c = (char)rnd[0];
    if ((Char.IsDigit(c) || Char.IsLetter(c)) && rnd[0] < 127) {
        ++n;
        coupon.Append(rnd[0]);
    }
}
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 < coupon.Length) {
    rng.GetBytes(rnd);
    rnd[0] %= 64;
    if (rnd[0] < 62) {
        ++n;
        coupon.Append((byte)((rnd[0] <= 9 ? '0' : rnd[0] <= 35 ? 'A' - 10 : 'a' - 36) + rnd[0]);
    }
}
\$\endgroup\$
5
\$\begingroup\$

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;
}
\$\endgroup\$
5
\$\begingroup\$

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

\$\endgroup\$
4
\$\begingroup\$

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.

\$\endgroup\$
  • 2
    \$\begingroup\$ +1, it's a good idea and the OP checks that the codes are unique (they're compared with former coupon codes), but I have to mention that "The chance that the value of the new Guid will be all zeros or equal to any other Guid is very low." msdn.microsoft.com/en-us/library/system.guid.newguid.aspx and the uniqueness of GUIDs isn't the same as uniqueness of the first then characters of GUIDs . \$\endgroup\$ – palacsint Nov 16 '11 at 21:00
  • \$\begingroup\$ Yes, the chance that any two GUIDs are non-unique is extremely small; however, it's (probably) going to be a bit larger if only part of it is being used. \$\endgroup\$ – Clockwork-Muse Nov 16 '11 at 23:50
  • \$\begingroup\$ thats an easy fix. If character case matters then he can set random characters to lower or upper case. This will solve the possiblity of collisions. I highly doubt he gets collisions unless he is pumping out guids one after the other on a super computer in a tight loop. \$\endgroup\$ – Luke101 Nov 17 '11 at 15:17
  • 1
    \$\begingroup\$ You don't need the 'Replace("-"...)` call, just do Guid.NewGuid().ToString("N"), that will format it with just hex characters, no dashes. \$\endgroup\$ – pstrjds Nov 17 '11 at 18:10
  • 1
    \$\begingroup\$ @Luke101 Bad solution. Changing 'random' characters to lower / upper case doesn't sound like a good 'fix' to me. You're only allowing 16 different options per character for a total of 16^10; OP is allowing 62 different options per character. You're only just using a quarter of the allowed characters for in position, or (1/4)^10 of the total solution space. You'll get overlaps far sooner than the OP's current solution. \$\endgroup\$ – Kirk Broadhurst Nov 17 '11 at 23:51
3
\$\begingroup\$

That Thread.Sleep(1) is a real problem, especially if you're going to use this to generate thousands or millions of coupons at a time. The only reason you need it is because you're creating a new instance of Random for each coupon and seeding that instance with the current time. The default constructor of Random already handles time-based seeding; if you make the instance static, you only need to construct it once and thus avoid the duplication issue.

I like @palacsint's idea of using a List<char> to store allowed characters and populating it with character-indexed for loops, although I'd make the list a lazy-initialized property rather than recreating it each time. And I fully agree with @Guffa's point about using StringBuilder to create the coupon rather than the += operator.

public class CouponGenerator
{
    private static List<char> _allowed = null;
    private static List<char> AllowedChars
    {
        get
        {
            if (_allowed == null)
            {
                _allowed = new List<char>();
                for (char c = 'A'; c < 'Z'; c++)
                {
                    _allowed.Add(c);
                }
                for (char c = 'a'; c < 'z'; c++)
                {
                    _allowed.Add(c);
                }
                for (char c = '0'; c < '9'; c++)
                {
                    _allowed.Add(c);
                }
            }
            return _allowed;
        }
    }
    private static Random _rg = new Random();
    public static string GenerateCoupon(int length)
    {
        StringBuilder sb = new StringBuilder(length);
        for (int i = 0; i < length; i++)
        {
            sb.Append(AllowedChars[_rg.Next(0, AllowedChars.Count)]);
        }
        return sb.ToString();
    }
}
\$\endgroup\$
3
\$\begingroup\$

I believe that the main problem is not the generation method but the uniqueness of the data.

As I understand there is a database and you have to check the generated data uniqueness retrospectively with your past data generated.

Suppose that there is a new requirement that your business side ask for another 100.000 unique data from you. And you have already generated 10.000.000 in the past. Which are stored in DB.

My suggestion will be;

1 - Generate for example 120.000 unique key ( greater than the requirement )

2 - Bulk insert those data to a temp table.

3 - Then execute a stored procedure to compare two tables in the SQL Server side,

4 - if your temp table contains 100.000 different value from your old 10.000.000 dataset, then you are good to go insert those 100.000 to your main table return them to your business side, else, for example 60.000 of 120.000 is different from your old data set but 60.000 is same, in this situation your stored procedure can return int = 40.000 which you will understand that you need another 40.000 data.

Go to the step 1, Execute your generator for another 60.000, 100.000 whatever you want and follow the same steps.

May not be the best solution but I believe will be fast. Because generating 1.000.000 of random alphanumeric strings take at most 2 seconds with the code below;

In the past I have used the below code to generate unique random data

public static string GetUniqueKey()
    {
        int size = 7;
        char[] chars = new char[62];
        string a = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890";
        chars = a.ToCharArray();

        RNGCryptoServiceProvider crypto = new RNGCryptoServiceProvider();

        byte[] data = new byte[size];
        crypto.GetNonZeroBytes(data);

        StringBuilder result = new StringBuilder(size);

        foreach (byte b in data)
            result.Append(chars[b % (chars.Length - 1)]);

        return Convert.ToString(result);
    }

and for the uniqueness of my runtime generated data, I have used a hashtable as can be seen above.

public static Tuple<List<string>, List<string>> GenerateUniqueList(int count)
    {
        uniqueHashTable = new Hashtable();
        nonUniqueList = new List<string>();
        uniqueList = new List<string>();

        for (int i = 0; i < count; i++)
        {
            isUniqueGenerated = false;

            while (!isUniqueGenerated)
            {
                uniqueStr = GetUniqueKey();
                try
                {
                    uniqueHashTable.Add(uniqueStr, "");
                    isUniqueGenerated = true;
                }
                catch (Exception ex)
                {
                    nonUniqueList.Add(uniqueStr);
                    // Non-unique generated
                }
            }
        }

        uniqueList = uniqueHashTable.Keys.Cast<string>().ToList();

        return new Tuple<List<string>, List<string>>(uniqueList, nonUniqueList);
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ If you want to merge your user accounts, you should use the [contact] form to ask for an account merger \$\endgroup\$ – Vogel612 Aug 21 at 11:58
2
\$\begingroup\$

Another way to do it:

    public static string CouponGenerator(int length)
    {
        var sb = new StringBuilder();
        for (var i = 0; i < length; i++)
        {
            var ch = Convert.ToChar(Convert.ToInt32(Math.Floor(26 * _random.NextDouble() + 65)));
            sb.Append(ch);
        }

        return sb.ToString();
    }
    private static readonly Random _random = new Random();
\$\endgroup\$
2
\$\begingroup\$

For my future self:

A singleton instance of @Guffa's implementation to avoid recreating StringBuilder and Random objects; less GC and a bit faster. Also @Gilles' implementation of GenerateUId beneath in case a crypto version is necessary.

public class UIdGenerator
{
    private static readonly Lazy<UIdGenerator> _lazy = new Lazy<UIdGenerator>(
        () => new UIdGenerator(), LazyThreadSafetyMode.ExecutionAndPublication);

    public static UIdGenerator Instance
    {
        get { return UIdGenerator._lazy.Value; }
    }

    private readonly Random _random = new Random();
    private readonly Dictionary<int, StringBuilder> _stringBuilders = new Dictionary<int, StringBuilder>();
    private const string CHARACTERS = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz";

    private UIdGenerator()
    {
    }

    public string GenerateUId(int length)
    {
        StringBuilder result;
        if (!_stringBuilders.TryGetValue(length, out result))
        {
            result = new StringBuilder();
            _stringBuilders[length] = result;
        }

        result.Clear();

        for (int i = 0; i < length; i++)
        {
            result.Append(CHARACTERS[_random.Next(CHARACTERS.Length)]);
        }

        return result.ToString();
    }
}

@Gilles' version:

// use me if you want a crypto version
public string GenerateUId(int length)
    // +6 to handle chances when value >= 62 (increase with fewer CHARACTERS to offset the probability of it occurring)
    int iterations = length + 6;

    StringBuilder result;
    if (!_stringBuilders.TryGetValue(length, out result))
    {
        result = new StringBuilder();
        _stringBuilders[length] = result;
    }
    result.Clear();

    // todo re-use like we're doing with the StringBuilder
    byte[] rnd = new byte[iterations];
    _crypto.GetBytes(rnd);
    int n = 0;
    for (int j = 0; j < iterations; ++j)
    {
        rnd[j] %= 64; 
        if (rnd[j] < 62)
        {
            coupon.Append(CHARACTERS[rnd[j]]);
            ++n;
            if (n == length)
            {
                break;
            }
        }
    }

    return result.ToString();
}

Usage:

UIdGenerator.Instance.GenerateUId(10);
\$\endgroup\$
1
\$\begingroup\$

Slight modification to Berrly's answer. Gives alphanumberic with casing and makes sure there are no duplicates created. I'm going to start using this for access tokens that get emailed to the user & text'd to log on to my system. This would be more secure than most of these systems today as they are more predictable as they are generally 6 characters long (fixed length) and numbers (limited set). Even though they all have a time limit on them why not go this extra mile as it's not that much harder.

Note: I remove similar looking characters to ease confusion when actually typing.

Side Rant: I believe with the advent of social login emailed/texted access tokens for granting access to other sites/apps will be the way to go. Nobody wants to remember all these passwords so they use the same one and we blame them, which is stupid because it's simply human nature. Then we make up password holders which you need a password to which is just 1 password holding all your other passwords and we think that's a good solution...The safest thing you can do is change passwords often and make them complicated but because we have to have so many nobody does this. We don't want to spend our lives changing passwords. Ideally we'd have the 1 password to say our email and everything goes through there for step 1 auth, then phone for step 2. People would be more willing to change their ONE password more often knowing it was just 1 and used to get access tokens that grant them access to all other apps/sites.

//Rextester.Program.Main is the entry point for your code. Don't change it.
//Compiler version 4.0.30319.17929 for Microsoft (R) .NET Framework 4.5

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using System.Text;

namespace Rextester
{
    public class Program
    {
        public static void Main(string[] args)
        {
            //Your code goes here
            Console.WriteLine("Hello, world!");

            var blackList = new List<char>{
                '[', ']', '`', '{', '}', '|', '\\', '/', '~', '^', '_', '-', ':', ';', '<', '>', '=', '?', '@', '0', 'o', 'l', 'I', '1'
            };

            var accessTokens = new List<string>();

            // test by creating a bunch of tokens
            for(int i = 0; i < 100; i++)
            {
                // this makes sure we don't duplicate active access tokens
                var token = CouponGenerator(_random.Next(6, 12), blackList);
                while(accessTokens.Contains(token)){
                    token = CouponGenerator(_random.Next(6, 12), blackList);
                }

                accessTokens.Add(token);
            }

            // print tokens out
            accessTokens.ForEach(x => Console.WriteLine("Access Token: " + x));
        }

        public static string CouponGenerator(int length, List<char> exclusionList)
        {
            var sb = new StringBuilder();
            for (var i = 0; i < length; i++)
            {
                var ch = Convert.ToChar(Convert.ToInt32(Math.Floor(62 * _random.NextDouble() + 48)));
                while(exclusionList.Contains(ch)){
                    ch = Convert.ToChar(Convert.ToInt32(Math.Floor(62 * _random.NextDouble() + 65)));
                }

                sb.Append(ch);
            }

            return sb.ToString();
        }
        private static readonly Random _random = new Random();
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.