HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMajor

Random string generation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomgenerationstring

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 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:

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
quWNakbybY

Code 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.