Code Review Asked by Brian Smithers on October 27, 2021
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;
}
}
}
Overall good implementation, well commented and easy to understand.
These are my suggestions:
InvalidValue()
method is never usedSystem.out.println
instead of System.out.printf
(no need of %n)System.exit(0)
the program ends automaticallypasswordGenerator
and pinGenerator
is better to return a String and print it rather than use System.out.print
inside (easier to reuse in the future)myArray2
in pinGenerator method? Maybe a typoThis 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 PINn2 - Create Passwordn3 - 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);
}
}
Answered by Marc on October 27, 2021
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.
Answered by user33306 on October 27, 2021
Get help from others!
Recent Answers
Recent Questions
© 2024 TransWikia.com. All rights reserved. Sites we Love: PCI Database, UKBizDB, Menu Kuliner, Sharing RPP