patterncsharpMinor
Efficient random code generator
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:
Your private fields (globals in your code) should be lower case:
The
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:
would become:
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.
And is there a specific reason why your code returns the
The initialisation of your arrays could also be made simpler:
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:
Example usage:
This can be a bit simplified by generating an extension method to get a random element from the array:
-
The extension method:
-
The method:
Naming conventions:
Please read the Capitalization Conventions by Microsoft. Method names will always be capitalized:
generateCode()becomesGenerateCode()
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.