How to get rid of duplicates from random name generator?

Question

I'm trying to make a random generator that selects a specific number of names from a list and pairs them together.

private void button1_Click(object sender, EventArgs e)
        {
            string[] lines = File.ReadAllLines(@"C:\Users\Brown\source\repos\Generator\bin\names.txt");

            List<string> source = new List<string>();
            int n = int.Parse(textBox1.Text);
            for (int i = 0; i < n; i++)
            {
                source.Add(lines[new Random().Next(lines.Length)]);
            }
            string joinNames = string.Join(" x ", source);

            genResult.Text = joinNames;
        }

I got it to work but when i click the generate button I see duplicates of some names.

You're using `new Random()` incorrectly. Only create a *single* instance of `Random`. — Dai, Mar 19 '21 at 02:08

Mehrdad Dowlatabadi · Answer 1 · 2021-03-19T03:45:48.053

1

To select n distinct names instead of the for loop just do this:

Random rnd=new Random();
source=lines.OrderBy(x=>rnd.Next()).Take(n);

Explanation: it will order the line elements in a random order then assigns the first n elements to source

The above solution is not optimal cause it uses the OrderBy method which currently(a Quicksort variant) executes in O(n*log(n)). You can use optimal solution using Fisher-Yates shuffle as introduced in Randomize a List

edited Mar 19 '21 at 03:45

answered Mar 19 '21 at 02:05

Mehrdad Dowlatabadi

1,335
2
9
11

1

Please review proper way of doing it https://stackoverflow.com/questions/273313/randomize-a-listt so you can provide better clones of that in the future (ideally you'd just vote as duplicate instead of suggesting suboptimal approach without corresponding waranings) – Alexei Levenkov Mar 19 '21 at 03:02

jcwmoore · Accepted Answer · 2021-03-19T02:26:27.560

1

Rather than loading your data into an array, you should use a collection like a List. Then as you randomly select items from the list, you should remove the record you just selected.

List<String> lines = File.ReadAllLines(@"C:\Users\Brown\source\repos\Generator\bin\names.txt").ToList();
int n = int.Parse(textBox1.Text);
Random rnd = new Random();
List<string> source = new List<string>();
for (int i = 0; i < n; i++)
{
    var tmp = lines[rnd.Next(lines.Count)];
    source.Add(tmp);
    lines.Remove(tmp);
}
string joinNames = string.Join(" x ", source);

genResult.Text = joinNames;

edited Mar 19 '21 at 02:26

answered Mar 19 '21 at 02:13

jcwmoore

1,153
8
13

Ya it good but creating new `new Random()` for every iteration is not a good idea. i think we can use same object. – Avinash Reddy Mar 19 '21 at 02:20
Agree, best to declare that outside of the main loop. – jcwmoore Mar 19 '21 at 02:25
Since comments here agree with something I agree that this is the second worst solution (the top one is to check if number already picked). – Alexei Levenkov Mar 19 '21 at 03:05

How to get rid of duplicates from random name generator?

2 Answers2