5
\$\begingroup\$

I am reaching my one year on programming and decided to create a simple pin and password program to better help me understand arrays. As far as coding standards, best practices, and functionality what else could I do to this to make it better?


public static void main(String []args){

    choicePrompt(); // Prompt to choose pin or password.
    menuMethod(); // Method to catch user input if not (1) PIN, (2) Password or (3) Exit.

}

static void choicePrompt() {
    System.out.printf("Password Generator:%n");
    System.out.printf("1 - Create PIN%n");
    System.out.printf("2 - Create Password%n");
    System.out.printf("3 - EXIT%n");
}

// Method accepts (1), (2), (3) or handles any input exception.
static void menuMethod() {
    int input = 0;  // Initialize integer input value.
    while (input != 1 && input != 2 && input != 3) { // User must choose these options to progress.
        Scanner obj = new Scanner(System.in); // Creates object for user input.
        try { // Try-catch block to catch incorrect input value...i.e. not an integer.
            input = obj.nextInt(); // 1 - PIN // 2 - Password // 3 - EXIT.
            if (input == 1 || input == 2 || input == 3) {
                inputMethod(input); // Successful input
            }
            else {
                invalidValuePrompt(); // Remind user to select correct value.
                menuMethod(); // Recursive call gives user option to create PIN, Password or EXIT.
            }
        } catch (InputMismatchException e) { // If input is not an integer.
            // Skip to finally block
        } finally {
            invalidValuePrompt(); // Remind user to select correct value.
            menuMethod(); // Recursive call gives user option to create PIN, Password or EXIT.
        }
    }
}

// Method for menuMethod()
static void invalidValuePrompt() {
    System.out.printf("Please enter: %n1 - Create PIN%n2 - Create Password%n3 - EXIT%n");
}

// Method takes users input and starts pinGenerator(), passwordGenerator() or EXITS program.
static void inputMethod(int input) { // If statement selects PIN, Password or EXIT option according to input.
    Scanner obj = new Scanner(System.in); // Creates object for user input.
    if (input == 1) { // PIN generator
        int pinLength = 0;
        System.out.printf("Enter pin length: (4 - 32)%n");
        try {
            pinLength = obj.nextInt();
            while (pinLength < 4 || pinLength > 32) {
                System.out.printf("Please enter: (4-32)%n");
                pinLength = obj.nextInt();
            }
        }  catch (InputMismatchException e) { // If input is not an integer.
            // Skip to finally block
        } finally { // Executes if user does not enter valid password length.
            if (pinLength < 4 || pinLength > 32) {
                invalidValuePrompt();
                inputMethod(input); // Recursive call to method.
            }
        }
        pinGenerator(pinLength); // PIN generates and is printed.
        System.exit(0); // Program is finished.
    }
    else if (input == 2) { // Password generator
        int passwordLength = 0;
        System.out.printf("Enter password length: (6-64)%n");
        try {
            passwordLength = obj.nextInt();
            while (passwordLength < 4 || passwordLength > 64) {
                invalidValuePrompt();
                System.out.printf("Please enter: (4-64)%n");
                passwordLength = obj.nextInt();
            }
        } catch (InputMismatchException e) {
            // Skip to finally block
        } finally {
            if (passwordLength < 4 || passwordLength > 64) {
                invalidValuePrompt();
                inputMethod(input); // Recursive call to method.
            }
        }
        passwordGenerator(passwordLength); // Password generates and is printed.
        System.exit(0); // Program exits.
    }
    else if (input == 3) { // EXIT program.
        System.exit(0);
    }
}

// Method used during try-catch blocks when user inputs a non-integer value.
static void invalidValue() {
    System.out.printf("Invalid value.%n");
}

// Method generates pin from an array. passwordLength is determined in previous
// method by user.
static void pinGenerator(int passwordLength) {
    SecureRandom random = new SecureRandom();
    int[] myArray2 = {1, 2, 3, 4, 5, 6, 7, 8, 9, 0};

    // Array prints random index with SecureRandom Class.
    for (int i = 0; i < passwordLength; i++) {
        int randomNum = random.nextInt(myArray2.length);
        System.out.print(myArray2[randomNum]);
    }
}

// Method generates a random password using SecureRandom class by utilizing four arrays with alphabetical
// characters, numerical values and special characters.
static void passwordGenerator(int passwordLength) {

    SecureRandom random = new SecureRandom();

    String[] myArray = {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j", "k", "l", "m", "n","o",
            "p", "q", "r", "s", "t", "u", "v", "w", "x", "y", "z"};
    int[] myArray2 = {1, 2, 3, 4, 5, 6, 7, 8, 9, 0};
    String[] myArray3 = {"A", "B", "C", "D", "E", "F", "G", "H", "I", "J", "K", "L", "M", "N", "O",
            "P", "Q", "R", "S", "T", "U", "V", "W", "X", "Y", "Z"};
    String[] myArray4 = {"!", "@", "#", "%", "&", "=", "?", "-"};

    // For loop generates password from array using switch class.
    for (int i = 0; i < passwordLength; i++) { // passwordLength equals users input.
        int valueChoice = 1 + random.nextInt(4); // Bounds account for all arrays in switch case.
        switch(valueChoice) {
            case 1: // Random lowercase letter is chosen from array.
                int randomLowerCaseLetter = random.nextInt(myArray.length);
                System.out.print(myArray[randomLowerCaseLetter]);
                break; // Restarts loop and generates next value.
            case 2: // Random numerical value (0-9) is chosen from array.
                int randomNum = random.nextInt(myArray2.length);
                System.out.print(myArray2[randomNum]);
                break;
            case 3: // Random uppercase letter is chosen from array.
                int randomUpperCaseLetter = random.nextInt(myArray3.length);
                System.out.print(myArray3[randomUpperCaseLetter]);
                break;
            case 4: // Random special character is chosen from array.
                int randomSymbols = random.nextInt(myArray4.length);
                System.out.print(myArray4[randomSymbols]);
                break;
        }
    }
}

\$\endgroup\$
3
\$\begingroup\$

Overall good implementation, well commented and easy to understand.

These are my suggestions:

  1. You can model the user choice as Enum (more readable)
  2. The code to request an integer from the user is duplicated many times, better to use a single method and reuse it
  3. InvalidValue() method is never used
  4. Better to use System.out.println instead of System.out.printf (no need of %n)
  5. No need to call System.exit(0) the program ends automatically
  6. For the methods passwordGenerator and pinGenerator is better to return a String and print it rather than use System.out.print inside (easier to reuse in the future)
  7. myArray2 in pinGenerator method? Maybe a typo
  8. As @tinstaafl suggested you can generate numbers and letters without using arrays (more readable)

This is the code refactored:

public static void main(String[] args) {
    choicePrompt(); // Prompt to choose pin or password.
    menuMethod(); // Method to catch user input if not (1) PIN, (2) Password or (3) Exit.
}

static void choicePrompt() {
    System.out.println("Password Generator:");
    System.out.println("1 - Create PIN");
    System.out.println("2 - Create Password");
    System.out.println("3 - EXIT");
}

// Method accepts (1), (2), (3)
static void menuMethod() {
    int input = requestIntBetween(1, 3, invalidValuePrompt());
    inputMethod(input);
}

static int requestIntBetween(int start, int end, String errorMessage) {
    int input = Integer.MIN_VALUE;
    Scanner obj = new Scanner(System.in);
    while (input < start || input > end) {
        if (obj.hasNextInt()) {
            input = obj.nextInt();
            if (input < start || input > end) {
                System.out.println(errorMessage);
            }
        } else {
            obj.nextLine();
            System.out.println(errorMessage);
        }
    }

    return input;
}

// Method for menuMethod()
static String invalidValuePrompt() {
    return "Please enter: \n1 - Create PIN\n2 - Create Password\n3 - EXIT";
}

// Method takes users input and starts pinGenerator(), passwordGenerator() or exits
static void inputMethod(int input) { 
    switch (Choice.from(input)) {
    case CREATE_PIN:
        System.out.println("Enter pin length: (4 - 32)");
        int pinLength = requestIntBetween(4, 32, "Please enter: (4-32)");
        String pin = pinGenerator(pinLength); // PIN generates and is printed.
        System.out.println(pin);
        break;
    case CREATE_PW:
        System.out.println("Enter password length: (6-64)");
        int passwordLength = requestIntBetween(6, 64, "Please enter: (6-64)");
        String pw = passwordGenerator(passwordLength); // Password generates and is printed.
        System.out.println(pw);
        break;
    case EXIT:
        // do nothing and exit
        break;
    }
}

// Method generates pin randomly. passwordLength is determined in previous
// method by user.
static String pinGenerator(int pinLength) {
    SecureRandom random = new SecureRandom();
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < pinLength; i++) {
        sb.append(random.nextInt(10));
    }
    return sb.toString();
}

// Method generates a random password using SecureRandom class by utilizing
// characters, numerical values and special characters.
static String passwordGenerator(int passwordLength) {
    SecureRandom random = new SecureRandom();
    String letters = "abcdefghijklmnopqrstuvwxyz";
    String[] specialCharacters = { "!", "@", "#", "%", "&", "=", "?", "-" };
    StringBuilder sb = new StringBuilder();
    // For loop generates password from array using switch class.
    for (int i = 0; i < passwordLength; i++) { // passwordLength equals users input.
        int valueChoice = 1 + random.nextInt(4); // Bounds account for all arrays in switch case.
        switch (valueChoice) {
        case 1: // Random lowercase letter
            sb.append(letters.charAt(random.nextInt(letters.length())));
            break; // Restarts loop and generates next value.
        case 2: // Random numerical value (0-9)
            sb.append(random.nextInt(10));
            break;
        case 3: // Random uppercase letter
            sb.append(letters.toUpperCase().charAt(random.nextInt(letters.length())));
            break;
        case 4: // Random special character is chosen from array.
            int randomSymbols = random.nextInt(specialCharacters.length);
            sb.append(specialCharacters[randomSymbols]);
            break;
        }
    }
    return sb.toString();
}

And the class Choice:

public enum Choice {
    CREATE_PIN(1), CREATE_PW(2), EXIT(3);

    private int value;

    Choice(int value) {
        this.value = value;
    }

    public int getValue() {
        return this.value;
    }

    public static Choice from(int choice) {
        return Stream.of(Choice.values())
                  .filter(c -> c.value == choice)
                  .findFirst().orElse(null);
    }
}
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Welcome to Code review. Nice first answer. \$\endgroup\$
    – Heslacher
    Jul 15 '20 at 8:53
  • 1
    \$\begingroup\$ I really appreciate your feedback. I like the way you changed the code. I had not thought about many of your changes and it certainly gives me much to think about. As far as the array aspect goes, I changed the names to make them more concise. I think I missed those towards the end when I was finishing up. I decided to use arrays to better familiarize myself with how to iterate through them. The invalidMenu() was meant to be used if the user did not put an integer value into the scanner. Thanks again. \$\endgroup\$ Jul 15 '20 at 12:16
  • 1
    \$\begingroup\$ Thank you, I'm happy you appreciated it, it's my first code review here. I believe there is still some room for improvements, for example, the PIN is just a numerical password, which means the two generators can be merged, even better in a proper class so you can reuse it easily. requestIntBetween is also better in its own class, so that the Scanner can be reused and closed properly. The main will just contain some configuration and the inputMethod. Please consider picking an answer when you are satisfied ;) \$\endgroup\$
    – Marc
    Jul 15 '20 at 13:43
3
\$\begingroup\$

A few things I noticed:

You're printing the password directly to the console. I would suggest, you get much more versatility by returning the password as a string. This way the user of the function can decide how or if the password gets displayed. For instance, displaying the password on a web page, or hashing it and storing it a database.

You haven't set any format limits on the password, just the length. Typically, besides the length, there are requirements for a minimum number of each type of character, capitals, lowercase, digits, and punctuation.

You wastefully repeat the array holding the digits for the pinGenerator function. In fact you can dispense with all the arrays except the one for punctuation. Simply set the max limit of random.nextInt to 26 or 10 and add 'a','A','0' to get the appropriate char.

I would suggest holding all the different types of characters in char[]'s. This way you can create a char[] for the password and create a string from that.

Likewise with your menu choices, expecting a char requires a lot less validating.

Instead of using a try-catch block and all those if-else blocks to validate the users choice(s), a simple switch will do the job much more easily. The try-catch block isn't made for this kind of validation and adds an extra degree of overhead to your code that isn't needed.

Only use 1 instance of the SecureRandom class as a private global field. I'ts wasteful to keep seeding a new instance every time the functions are run.

\$\endgroup\$
3
  • \$\begingroup\$ Thank you, I appreciate your feedback. I implemented the integer array by itself last night. I forgot that you could use a switch and a default branch to work around the try-catch. If I remember correctly now, a switch is a better option since it's only looking at one variable. Could you clarify the need for returning the password as a string? What would be a good example of returning it as a string vs my current implementation? \$\endgroup\$ Jul 15 '20 at 12:09
  • 1
    \$\begingroup\$ @BrianSmithers - 2 possibilities come to mind, displaying it in a web page, or hashing it and storing it in a database. \$\endgroup\$
    – user33306
    Jul 15 '20 at 12:16
  • \$\begingroup\$ Okay, that makes plenty of sense. I have not got to that level of programming yet but it's good to know for future use. Thanks. \$\endgroup\$ Jul 15 '20 at 12:18

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.