CD Generator won't work

Question

the output for my key generator is the same for each part of the key even though I call GetLetter multiple times which should return different results. Any ideas? Much appreciated.

Output (for example) : B1J2-B1J2-B1J2-B1J2

    private void btn_generate_Click(object sender, EventArgs e) {
        txt_generate.Text = Generate();
    }

    public string Generate() {

        string[] code = new string[4];
        Random number = new Random();

        for (int i =0; i < 4; i++) {
             code[i] = GetLetter();
        }


        string code1 = code[0] + "-" + code[1] + "-" + code[2] + "-" + code[3];

        return code1;

    }

    public string GetLetter() {

        Random number = new Random();

        const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";

        string newWord = "";

        for(int i = 0; i < 4; i++) {
            char temp = chars.ElementAt(number.Next(0, 36));

            newWord += temp.ToString();

        }

        return newWord;
    }
}

}

Just out of interest why dont you use `Guid.NewGuid().ToString()`? — Nico, May 17 '17 at 23:58

score 1 · Accepted Answer · answered May 17 '17 at 23:56

When you call Random number = new Random(); new object is created and random is seeded with some value (usually current time).

As soon as your code executes quite fast, you're creating this object 4 times, but every time seed is the same. This is why all chunks are the same.

You can fix it in two ways:

Move number variable initialization outside of the method, make it static property of the class
Generate all 16 chars without random re-init and then split final string into 4 symbol chunks

Oh yes I see now, how did I miss that. Thanks! :) – Ralf May 18 '17 at 00:02 — Ralf, May 18 '17 at 00:02

bashis · Answer 2 · 2017-05-18T11:34:43.577

0

Refactored it a bit for you:

private void btn_generate_Click(object sender, EventArgs e) {
    txt_generate.Text = Generate();
}

private static Random _random = new Random();

public string Generate() 
{
    return string.Join("-", Enumerable.Range(0, 4).Select(i => GetLetter()));
}

public string GetLetter() 
{
    const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";

    return new string(Enumerable.Range(0, 4).Select(i => chars[_random.Next(chars.Length)]).ToArray());
}

edited May 18 '17 at 11:34

answered May 17 '17 at 23:55

bashis

1,200
1
16
35

I do love LINQ, thank you for that. – Ralf May 18 '17 at 00:07
I get this error when returning new the string from the GetLetter function - Error CS1503 Argument 1: cannot convert from 'System.Collections.Generic.IEnumerable' to 'char*' – Ralf May 18 '17 at 10:51
@Ralf fixed it, thank you – bashis May 18 '17 at 11:34

score 0 · Answer 3 · answered May 17 '17 at 23:56

The issue comes from creating a new Random instance everytime you call GetLetter and in Generate. Instead you should create a static Random in the class such as.

static Random number = new Random();

private void btn_generate_Click(object sender, EventArgs e)
{
    txt_generate.Text = Generate();
}

public string Generate()
{

    string[] code = new string[4];
    for (int i = 0; i < 4; i++)
    {
        code[i] = GetLetter();
    }


    string code1 = code[0] + "-" + code[1] + "-" + code[2] + "-" + code[3];

    return code1;

}

public string GetLetter()
{
    const string chars = "ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789";
    string newWord = "";

    for (int i = 0; i < 4; i++)
    {
        char temp = chars.ElementAt(number.Next(0, 36));

        newWord += temp.ToString();

    }

    return newWord;
}

Which would be better, to make the Random object a static member of the class or put the Random object into the Generate function and pass it into GetLetter as a parameter? Both work now, just curious if it's bad practice. — Ralf, May 18 '17 at 00:05
I would use a static object myself as it will be generated from a single seed and then reused for every subsequent call. Therefore if you have multiple requests to the same methods you will have a less likely chance of duplicate results. — Nico, May 18 '17 at 00:08

CD Generator won't work

3 Answers3