4
\$\begingroup\$

I made Password Generator on C#.

It prompts the user for the length of the password, whether it contains numbers, lowercase letters, uppercase letters or symbols, and generates a strong password from randomly added numbers, letters, or symbols.

There's the code:

using System;

namespace Password_Generator
{
    class Program
    {
        static string Randomize(string Str)
        {
            Random Rand = new Random();

            string Randomized = "";

            while (Randomized.Length < Str.Length)
            {
                Randomized += Str[Rand.Next(0, Str.Length)];
            }

            return Randomized;
        }

        static void Main(string[] args)
        {
            while (true)
            {
                Console.ForegroundColor = ConsoleColor.White;

                Console.Write("\nDo Yout Want a New Password [True], [False]? ");
                Console.ForegroundColor = ConsoleColor.Yellow;
                string NewPasswordStr = Console.ReadLine();
                bool NewPassword;

                while (bool.TryParse(NewPasswordStr, out NewPassword) == false)
                {
                    Console.Clear();

                    Console.ForegroundColor = ConsoleColor.Red;
                    Console.WriteLine("The Given Output is Unconvertable Or it's Value is Unacceptable. Try Again!");

                    Console.ForegroundColor = ConsoleColor.White;
                    Console.Write("\nDo Yout Want a New Password [True], [False]? ");

                    Console.ForegroundColor = ConsoleColor.Yellow;
                    NewPasswordStr = Console.ReadLine();
                    Console.ForegroundColor = ConsoleColor.White;
                }

                Console.ForegroundColor = ConsoleColor.White;

                if (NewPassword == true)
                {
                    Console.Clear();

                    Console.Write("Password Length: ");

                    Console.ForegroundColor = ConsoleColor.Yellow;
                    string LenghtStr = Console.ReadLine();
                    int Lenght;

                    while (int.TryParse(LenghtStr, out Lenght) == false || int.Parse(LenghtStr) <= 0)
                    {
                        Console.Clear();

                        Console.ForegroundColor = ConsoleColor.Red;
                        Console.WriteLine("The Given Output is Unconvertable Or it's Value is Unacceptable. Try Again!");

                        Console.ForegroundColor = ConsoleColor.White;
                        Console.Write("\nPassword Length: ");

                        Console.ForegroundColor = ConsoleColor.Yellow;
                        LenghtStr = Console.ReadLine();
                        Console.ForegroundColor = ConsoleColor.White;
                    }

                    Console.ForegroundColor = ConsoleColor.White;

                    Console.Write("\n\nInclude Numbers (0 - 9) [True], [False]: ");

                    Console.ForegroundColor = ConsoleColor.Yellow;
                    string IncludeNumbersStr = Console.ReadLine();
                    bool IncludeNumbers;

                    while (bool.TryParse(IncludeNumbersStr, out IncludeNumbers) == false)
                    {
                        Console.ForegroundColor = ConsoleColor.Red;
                        Console.WriteLine("\nThe Given Output is Unconvertable. Try Again!");

                        Console.ForegroundColor = ConsoleColor.White;
                        Console.Write("\nInclude Numbers (0 - 9) [True], [False]: ");

                        Console.ForegroundColor = ConsoleColor.Yellow;
                        IncludeNumbersStr = Console.ReadLine();
                        Console.ForegroundColor = ConsoleColor.White;
                    }

                    Console.Write("\n\nInclude Lowercase Letters (a - z) [True], [False]: ");

                    Console.ForegroundColor = ConsoleColor.Yellow;
                    string IncludeLowercaseStr = Console.ReadLine();
                    bool IncludeLowercase;

                    while (bool.TryParse(IncludeLowercaseStr, out IncludeLowercase) == false)
                    {
                        Console.ForegroundColor = ConsoleColor.Red;
                        Console.WriteLine("\nThe Given Output is Unconvertable. Try Again!");

                        Console.ForegroundColor = ConsoleColor.White;
                        Console.Write("\nInclude Lowercase Letters (a - z) [True], [False]: ");

                        Console.ForegroundColor = ConsoleColor.Yellow;
                        IncludeLowercaseStr = Console.ReadLine();
                        Console.ForegroundColor = ConsoleColor.White;
                    }

                    Console.ForegroundColor = ConsoleColor.White;

                    Console.Write("\n\nInclude Uppercase Letters (A - Z) [True], [False]: ");

                    Console.ForegroundColor = ConsoleColor.Yellow;
                    string IncludeUppercaseStr = Console.ReadLine();
                    bool IncludeUppercase;

                    while (bool.TryParse(IncludeUppercaseStr, out IncludeUppercase) == false)
                    {
                        Console.ForegroundColor = ConsoleColor.Red;
                        Console.WriteLine("\nThe Given Output is Unconvertable. Try Again!");

                        Console.ForegroundColor = ConsoleColor.White;
                        Console.Write("\nInclude Uppercase Letters (A - X) [True], [False]: ");

                        Console.ForegroundColor = ConsoleColor.Yellow;
                        IncludeUppercaseStr = Console.ReadLine();
                        Console.ForegroundColor = ConsoleColor.White;
                    }

                    Console.ForegroundColor = ConsoleColor.White;

                    Console.Write("\n\nInclude Symbols [True], [False]: ");
                    Console.ForegroundColor = ConsoleColor.Yellow;
                    string IncludeSymbolsStr = Console.ReadLine();
                    bool IncludeSymbols;

                    while (bool.TryParse(IncludeSymbolsStr, out IncludeSymbols) == false)
                    {
                        Console.ForegroundColor = ConsoleColor.Red;
                        Console.WriteLine("\nThe Given Output is Unconvertable. Try Again!");

                        Console.ForegroundColor = ConsoleColor.White;
                        Console.Write("\nInclude Symbols [True], [False]: ");

                        Console.ForegroundColor = ConsoleColor.Yellow;
                        IncludeSymbolsStr = Console.ReadLine();
                        Console.ForegroundColor = ConsoleColor.White;
                    }

                    Console.ForegroundColor = ConsoleColor.White;

                    string Password = "";

                    while (Password.Length < Lenght)
                    {
                        Random Rand = new Random();

                        if (IncludeNumbers == true)
                        {
                            Password += ((char)Rand.Next(48, 58)).ToString();
                        }

                        if (Password.Length == Lenght)
                        {
                            break;
                        }

                        if (IncludeLowercase == true)
                        {
                            Password += ((char)Rand.Next(97, 123)).ToString();
                        }

                        if (Password.Length == Lenght)
                        {
                            break;
                        }

                        if (IncludeUppercase == true)
                        {
                            Password += ((char)Rand.Next(97, 123)).ToString().ToUpper();
                        }

                        if (Password.Length == Lenght)
                        {
                            break;
                        }

                        if (IncludeSymbols == true)
                        {
                            Password += ((char)Rand.Next(33, 48)).ToString().ToUpper();
                        }

                        if (Password.Length == Lenght)
                        {
                            break;
                        }
                    }

                    Password = Randomize(Password);

                    Console.WriteLine();

                    Console.ForegroundColor = ConsoleColor.DarkGreen;
                    Console.WriteLine(Password);
                    Console.ForegroundColor = ConsoleColor.White;
                }

                else
                {
                    Environment.Exit(-1);
                }
            }

        }
    }
}

Are 221 line too much for this? How to impove it? Is this a "crapcode"? How do not write crapcode? ....Thanks!

\$\endgroup\$
7
\$\begingroup\$

From MSDN Random class documentation

To generate a cryptographically secure random number, such as one that's suitable for creating a random password, use the RNGCryptoServiceProvider class or derive a class from System.Security.Cryptography.RandomNumberGenerator.


Random Rand = new Random();

Edit Correction: Using a snapshot of the system clock, the no-parameter constructor ends up using different seed values; unless the Random objects are created so close in time that the same clock time is used. Given the same argument (parameter seed value), different Randoms generate the same random sequence. end Edit

Every Random object created with the parameterless constructor will generate the same sequence of random numbers because the same "default seed" value is used every time. This is perfect for development and testing but not in production code. Reading the system time in milliseconds is a common way to generate different "seed" values for each constructor call.


crapcode Code Smell

The fuzzy doubts you have about the code is technically called a "code smell", meaning it looks suspicious for some reason - the length "feels" longish for what it is doing, in this case. There may be something wrong, maybe not; a "code smell" requires closer scrutinization to determine if anything is in fact wrong.

A code smell I see is the multiple similar looking while loops, each looking for different required characters. I don't want to write hard to understand code for the sake of fewer lines. "Fewer lines of code" alone is not a reason to change things. Here is an idea, maybe it is an improvement or maybe not:

Tell the user the required characters up front when prompting for the password.

A string is iterable, you can "forEach". A String is in fact an array of Char, technically.

forEach (char character in enteredPassword) { ... }

Examine each character for its type. Keep a separate count for each character type.

After the forEach test each counter for the required minimum. If symbols is zero, for example, append "at least one symbol character is required" to an error message.


FYI

There's an old saying with a core of truth: If you solve a problem with a regular expression, now you have two problems


Are 221 line too much for this

There are many guidelines, principles, "isms", etc. for evaluating code quality. The line count is not one of them.

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Your comments about the behaviour of the parameterless Random constructor are incorrect. On the full framework, it uses the system clock to get a seed. That means you can have trouble if you create 2 instances at the same time. The behaviour on .Net Core is different again. \$\endgroup\$ – RobH Sep 28 at 13:32
  • \$\begingroup\$ OOps! corrected in answer. \$\endgroup\$ – radarbob Sep 28 at 20:59
  • \$\begingroup\$ Good edit, I can +1 now :) \$\endgroup\$ – RobH Sep 29 at 7:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.