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

Efficient random code generator

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

Problem

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();
    }

Solution

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);
}

Code Snippets

string[] RL = new string[9];
var RL = new string[9];
private string GenerateCode()
{
    //...
}

private void Generate_Click(object sender, RoutedEventArgs e)
{
    var code = GenerateCode();
    YourTextBox.Text = code;
}
private readonly char[] letters= "ABCDEFGHIJKLMNOPQRSTUVWXYZ".ToCharArray();
private readonly char[] numbers = "01234567489".ToCharArray();
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);
}

Context

StackExchange Code Review Q#95571, answer score: 7

Revisions (0)

No revisions yet.