Efficient random code generator

Question

I made a 16 character code generator. It works like a charm now, but I was wondering if there's any way to do it more efficiently.

public partial class MainWindow : Window
{

    //Globals
    string[] Letters = new string[27] { "A", "B", "C", "D", "E", "F", "G", "H", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"};
    string[] Numbers = new string[9] { "1", "2", "3", "4", "5", "6", "7", "8", "9" };

    Random r = new Random();
    int i = 0;
    int a = 0;

    private string[] generateCode()
    {

        string[] RL = new string[9];
        string[] RN = new string[9];

        //Random letters
        while (i < 9)
        {
            RL[i] = Letters[r.Next(0, Letters.Length - 1)];
            i++;
        }
        //Random numbers
        while (a < 9)
        {
            RN[a] = Numbers[r.Next(0, Numbers.Length - 1)];
            a++;
        }

        TextBox.Text = String.Format("{0}{1}{2}{3}-{4}{5}{6}{7}-{8}{9}{10}{11}-{12}{13}{14}{15}", RN[0], RL[1], RN[1], RL[2], RN[2], RL[3], RN[3], RL[4], RN[4], RL[5], RN[5], RL[6], RN[6], RL[7], RN[7], RL[8], RN[8]);

        //Resetting the while loop
        i = 0;
        a = 0;

        //Clipboard
        Clipboard.SetText(TextBox.Text);
        Message.Content = ("Copied to clipboard!");

        return RL;
    }

    public MainWindow()
    {
        InitializeComponent();
    }

    private void Generate_Click(object sender, RoutedEventArgs e)
    {
        generateCode();
    }

Answer 1

i and a shouldn't be class members, they should be declared in your method scope. That is because they are specifically used for your method implementation. This way, you won't need to reset them afterwards. Some people already pointed out that you should use the C# code conventions (generateCode should be GenerateCode).

You don't need to use while loops in this case. You want to do an operation X times, and the for is all designed for this. Also, there is no need for the 2 loops, only one could do the job.

RN and RL are quite confusing variable names, I understand that they are in fact randomNumbers and randomLetters, I think these names are clearer!

We see the int 9 used alot in the code, what if I wanted to change it to... 12. I'd have to check all the code and change all the instances of 9 for 12. This looks like a good scenario to use a constant!

Your method GenerateCode should have one simple responsability, to generate a code. So, this method shouldn't worry about the TextBox nor the ClipBoard. This method should return the generated code, and your Generate_Click method should take care of the rest.

It is a good practice, if I recall, to use a static Random variable, in order to have a better... randomability (I just invented that).

I'm not sure if this is a best practice or a personnal preference, but I like to see the private visibility modifier indicated, even if by default it is private. This way I can make sure it is intended to be private and that the developer didn't simply forget it.

So right now, this is what the code looks like :

private const int NumberOfLettersOrNumbers = 9;

private string[] letters = new string[27] { "A", "B", "C", "D", "E", "F", "G", "H", "H", "I", "J", "K", "L", "M", "N", "O", "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"};
private string[] numbers = new string[9] { "1", "2", "3", "4", "5", "6", "7", "8", "9" };

private static Random r = new Random();

private static string GenerateCode()
{
    string[] randomLetters = new string[NumberOfLettersOrNumbers];
    string[] randomNumbers = new string[NumberOfLettersOrNumbers];

    for(int i = 0; i < NumberOfLettersOrNumbers; i++)
    {
        randomLetters[i] = letters[r.Next(0, letters.Length - 1)];
        randomNumbers[i] = numbers[r.Next(0, numbers.Length - 1)];
    }

    return String.Format("{0}{1}{2}{3}-{4}{5}{6}{7}-{8}{9}{10}{11}-{12}{13}{14}{15}", randomNumbers[0], randomLetters[1], randomNumbers[1], randomLetters[2], randomNumbers[2], randomLetters[3], randomNumbers[3], randomLetters[4], randomNumbers[4], randomLetters[5], randomNumbers[5], randomLetters[6], randomNumbers[6], randomLetters[7], randomNumbers[7], randomLetters[8], randomNumbers[8]);
}

private void Generate_Click(object sender, RoutedEventArgs e)
{
    string generatedCode = generateCode();
    TextBox.Text = generatedCode;

    //Clipboard
    Clipboard.SetText(generatedCode);
    Message.Content = ("Copied to clipboard!");
}

Now, if we want to get rid of the String.Format that is a little difficult to read, and keep the fact that you alternate between a number and a letter in your code, we will try and use a StringBuilder instead of 2 string[]

private const int CodeSize = 16;
private const int GroupSize = 4;
private const string GroupSeparator = "-";

private static string GenerateCode()
{
    StringBuilder code = new StringBuilder();

    while(code.Length < CodeSize)
    {
        code.Append(letters[r.Next(0, letters.Length - 1)]);
        code.Append(numbers[r.Next(0, numbers.Length - 1)]);
    }

    //Note the "+1" at the end of the loop, because we want to separate after the 4th character, so the 4th index, not the 3rd.
    for(int separatorIndex = GroupSize; separatorIndex < code.Length; separatorIndex += GroupSize + 1)
    {
        code.Insert(separatorIndex, GroupSeparator);
    }

    return code.ToString();
}

The code right above constructs your code in 2 times, the first where we will add one number and one letter one after another until we reach our CodeSize. The second part will have the responsibility to add your separators. The real advantage is that you don't need to worry anymore about your array's indexes, which could become confusing.

Now, if we want to remove the second loop, we need to be able to tell when to Append the GroupSeparator at the good moment in the first while. To do so, we need to be able to tell how long will the final code, including the separators, will be. Let's introduce another constant to do the maths!

private const int SizeWithSeparator = CodeSize + (CodeSize - 1) / GroupSize;

Since we don't want a separator at the end of the code, we need to do codeSize - 1, and we need to see how many separators will be neccessary, hence the CodeSize / GroupSize.

private const int CodeSize = 16;
private const int GroupSize = 4;
private const int SizeWithSeparator = CodeSize + (CodeSize - 1) / GroupSize;
private const string GroupSeparator = "-";

private string GenerateCode()
{
    var code = new StringBuilder();
    int separatorIndex = 0;

    while(code.Length < SizeWithSeparator)
    {
        //We need the second section of the condition to not have a separator at index 0.
        if(separatorIndex % GroupSize == 0 && separatorIndex != 0)
        {
            code.Append(GroupSeparator);
        }

        code.Append(letters[r.Next(0, letters.Length - 1)]);
        code.Append(numbers[r.Next(0, numbers.Length - 1)]);

        //We just added 2 characters to our code.
        separatorIndex += 2;
    }

    return code.ToString();
}

We need to check the condition before incrementing the separatorIndex in order not to have a separator at the end of the code. Also, note that I used the var keyword as @Abbas proposed, it is a great keyword to use when it is easy to guess what is the type!

This last step might not be a good idea, but I decided I was gonna share it with you anyway. You do not need the 2 arrays, thanks to the ASCII table. As you might know, 'A' = 65. Now, as I wrote before, using hardcoded values in the code isn't good, and I'm pretty sure you don't want to introduce constants like :

const AAscii = 65; 
const ZAscii = 90;

So we will extract this logic in a method, in order to hide to your method these ugly hardcoded values.

private char GetRandomLetter()
{
    //65 == "A"
    //91 == "Z" + 1
    return (char)r.Next(65,91);
}

So it would look like this :

private const int CodeSize = 16;
private const int GroupSize = 4;
private const int SizeWithSeparator = CodeSize + (CodeSize - 1) / GroupSize;
private const string GroupSeparator = "-";

private static Random r = new Random();

private char GetRandomLetter()
{
    //65 == "A"
    //91 == "Z" + 1
    return (char)r.Next(65,91);
}

private string GenerateCode()
{
    var code = new StringBuilder();
    int separatorIndex = 0;

    while(code.Length < SizeWithSeparator)
    {
        //We need the second section of the condition to not have a separator at index 0.
        if(separatorIndex % GroupSize == 0 && separatorIndex != 0)
        {
            code.Append(GroupSeparator);
        }

        code.Append(GetRandomLetter());
        code.Append(r.Next(1,10).ToString());

        //We just added 2 characters to our code.
        separatorIndex += 2;
    }

    return code.ToString();
}

But wait, there's more! At the moment, we have constant values that define our code rules. 16 characters, groups of 4 and a "-" separator. These values could (should?) be parameters to an object!

So let's introduce the CodeGenerator class, with a static method, Generate.

public class CodeGenerator
{
    private static Random r = new Random();

    private static char GetRandomLetter()
    {
        //65 == "A"
        //91 == "Z" + 1
        return (char)r.Next(65,91);
    }

    public static string Generate(int characterCount, int groupSize, string separator = "-")
    {
        int sizeWithSeparator = characterCount + (characterCount - 1) / groupSize;
        var code = new StringBuilder();
        var separatorIndex = 0;

        while(code.Length < sizeWithSeparator)
        {
            //We need the second section of the condition to not have a separator at index 0.
            if(separatorIndex % groupSize == 0 && separatorIndex != 0)
            {
                code.Append(separator);
            }

            code.Append(GetRandomLetter());
            code.Append(r.Next(1,10).ToString());

            //We just added 2 characters to our code.
            separatorIndex += 2;
        }

        return code.ToString();
    }
}

Now, you can reuse this code generator for other codes. The separator is an optional parameter. I don't know if you already know about this, but if not, here's the link : https://msdn.microsoft.com/en-us/library/vstudio/dd264739(v=vs.100).aspx

Answer 2

First a bit of nitpicking:

Naming conventions:

Please read the Capitalization Conventions by Microsoft. Method names will always be capitalized:

• generateCode() becomes GenerateCode()

Your private fields (globals in your code) should be lower case: letters and numbers.

The var keyword:

From the C# Programming Guide:

The var keyword can also be useful when the specific type of the variable is tedious to type on the keyboard, or is obvious, or does not add to the readability of the code.

So lines like:

string[] RL = new string[9];

would become:

var RL = new string[9];

---

Separation of concerns:

From the method that generates the code, you set the value of the textbox. This is not a good practice reagarding maintenance of your code. What if you want to use that method somewhere else?

Assign the value of the method to a variable in the click event of the button and assign that value to the textbox.

private string GenerateCode()
{
    //...
}

private void Generate_Click(object sender, RoutedEventArgs e)
{
    var code = GenerateCode();
    YourTextBox.Text = code;
}

And is there a specific reason why your code returns the RL array?

---

The initialisation of your arrays could also be made simpler:

private readonly char[] letters= "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();
private readonly char[] numbers = "01234567489".ToCharArray();

The answer of austin wernli already provides a nice solution, I have written another way of achieving this. This method uses only one loop and no if statements:

private readonly char[] _letters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();
private readonly char[] _numbers = "01234567489".ToCharArray();
private readonly Random _random = new Random();

private string GenerateCode()
{
    var parts = new string[4];

    for (var i = 0; i < 4; i++)
    {
        parts[i] = String.Format("{0}{1}{2}{3}", _numbers[_random.Next(0, _numbers.Length)], _letters[_random.Next(0, _letters.Length)], _numbers[_random.Next(0, _numbers.Length)], _letters[_random.Next(0, _letters.Length)]);
    }

    return String.Join("-", parts);
}

Example usage:

private void Generate_Click(object sender, RoutedEventArgs e)
{
    YourTextBox.Text = GenerateCode();
}

This can be a bit simplified by generating an extension method to get a random element from the array:

• The extension method:

  public static class Extensions
  {
      private static readonly Random Random = new Random();
  
      public static char GetRandom(this char[] collection)
      {
          return collection[Random.Next(0, collection.Length)];
      }
  }
  

• The method:

  private string GenerateCode()
  {
      var parts = new string[4];
  
      for (var i = 0; i < 4; i++)
      {
          parts[i] = String.Format("{0}{1}{2}{3}", _numbers.GetRandom(), _letters.GetRandom(), _numbers.GetRandom(), _letters.GetRandom());
      }
  
      return String.Join("-", parts);
  }
  

Answer 3

You could shorten up some pieces of your code and use loops to generate your code instead of string.Format

To make the loops more efficient, you can use a StringBuilder So that you do not need to create a new string every time with string concatenation.

You can also shorten up your initial Number and Letter declarations using string.ToCharArray

Also, I didn't see anywhere you were using the return RL; in the code you have provided.

This code is not tested by the way, so there may be some bugs in it.

public partial class MainWindow: Window {

    char[] Letters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();
    char[] Numbers = "123456789".ToCharArray();

    Random r = new Random();

    private void generateCode() {

        var codeBuilder = new StringBuilder();
        var codeLength = 16;
        for (var i = 0; i < codeLength; i++) {
            for (var j = 0; j < 4; j++) {
                codeBuilder.Append(Numbers[r.Next(0, Numbers.Length - 1]));
                codeBuilder.Append(Letters[r.Next(0, Letters.Length - 1]));
                codeBuilder.Append(Numbers[r.Next(0, Numbers.Length - 1]));
                codeBuilder.Append(Letters[r.Next(0, Letters.Length - 1]));
            }
            if (i < codeLength - 1) {
                codeBuilder.Append("-");
            }
        }

        var codeString = codeBuilder.ToString();

        TextBox.Text = codeString;
        Clipboard.SetText(codeString);

        Message.Content = ("Copied to clipboard!");
    }

    public MainWindow() {
        InitializeComponent();
    }

    private void Generate_Click(object sender, RoutedEventArgs e) {
        generateCode();
    }