-2

I have this code which should creates a sequence of random letters and adds "-" every 5th term (i.e.: AxDG-aGJA-fbnz-KASz)

string pw;
string letters = "qwertyuiopasdfghjklzxcvbnmQWERTYUIOPASDFGHJKLZXCVBNM";
char?[] pwArray = new char?[19];
Random r = new Random();
for (int iter = 0; iter < 19; iter++)
{
    pwArray[iter] = letters[r.Next(letters.Length)];
    if (iter == 3 || iter == 9 || iter == 13)
    {
        pwArray[pwArray.Count(c => c != null) + 1] = '-';
        iter+=1;
    }
}

pw = String.Join(null, pwArray);

It works fine for the the first 4 iterations (0-3) then it "somewhat" breaks and creates a sequence that looks like this: nYWuSwhrt-Kkc-DBOy

I tried changing the if part and the iter inside the if part and I am desperate to understand why it simply breaks.

5
  • Use a string builder to avoid array math Commented Mar 5, 2024 at 14:42
  • You are checking for index 3, 9 and 13 which would be 6 and 4 apart, not every 5th term. Commented Mar 5, 2024 at 14:54
  • pwArray.Count(c => c != null) <- this is too shocking, do you know that you can just set hyphen like this: pwArray[iter + 1] = '-'? Commented Mar 5, 2024 at 15:00
  • Thank you for the feedback and corrections! I guess I was too tired and thought of changing those and I guess they went over my head. Thank you again!!! Commented Mar 5, 2024 at 15:15
  • Does this answer your question? Format string with dashes Commented Mar 5, 2024 at 15:26

1 Answer 1

3

Making the chars in the array nullable and then later replacing the null chars by '-' seems over complicated. Instead, insert them directly when required. The loop variable is the very array index we need and there is no point in counting characters.

const string PasswordCharacters = "qwertyuiopasdfghjklzxcvbnmQWERTYUIOPASDFGHJKLZXCVBNM";
const int ChunkLength = 4, NumberOfChunks = 4;
const int TotalPassordLength = ChunkLength * NumberOfChunks + NumberOfChunks - 1;

char[] pwArray = new char[TotalPassordLength];
Random r = new Random();
for (int i = 0; i < pwArray.Length; i++) {
    if ((i + 1) % (ChunkLength + 1) == 0) {
        pwArray[i] = '-';
    } else {
        pwArray[i] = PasswordCharacters[r.Next(PasswordCharacters.Length)];
    }
}

string pw = new String(pwArray);

Console.WriteLine(pw);
Console.ReadKey();

prints e.g.:

iDAu-Fcqi-CBdB-zyRp

It is easier to delegate all the calculations to our code by declaring some constants. This also makes it very easy to change these parameters in the future. Note that expressions involving only constants are calculated at compile time. Therefore they will not have a negative impact at runtime. And even if they would - we are talking about nanoseconds. So, prefer a robust code over unnecessary optimization.

The modulo operator % returns the remainder of an integer division. Since we do not want to insert a '-' at position 0, I calculate (i + 1) % 5, so that the first index with an i + 1 divisible by 5 (i.e., with a remainder of 0) will be 4 and then 9 and 14.

Using pwArray.Length in the loop condition instead of the hard-coded 19 has the advantage that the condition will automatically adapt to any array length changes. But since I introduced a constant for the password length, we could also use this constant instead.

System.String (with the C# alias string) has a constructor accepting a char array. This is more performing than String.Join.

I renamed letters to PasswordCharacters and made it a constant. The new name allows you to add other characters like digits without having to change the name. Btw., the order of the characters in this string does not matter, as we pick them randomly anyway. So, you could have written them in alphabetical order without making the resulting password less random.

Sign up to request clarification or add additional context in comments.

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.