patternjavaMinor
Command line password generator
Viewed 0 times
commandgeneratorlinepassword
Problem
I built a random password generator in Java which is meant to be run from the command line. It accepts options for:
The project is on BitBucket.
Example program execution :
This generates a 12 character long password with upper and lower case letters, numbers, and special characters.
What I think :
Parser.java
```
package passManager;
import java.util.ArrayList;
import java.util.Arrays;
public class Parser {
public static ArrayList makeArgList(String args[]) {
ArrayList argList = new ArrayList();
boolean allArgsValid = true;
// Checks if each arg is valid
for (int i = 0; i < args.length && allArgsValid; i++) {
// If it starts with - and is not in the list of parameters, bad
if (args[i].startsWith("-") && !isInArgumentsList(args[i], Constants.VALID_ARGS)) {
System.out.println("Invalid parameter: " + args[i] + ". Exiting.");
allArgsValid = false;
// If it's not a parameter and it isn't a number, bad
} else if (!isInArgumentsList(args[i], Constants.VALID_ARGS) && !args[i].matches("^\\d+$")) {
System.out.println("Invalid parameter: " + args[i] + ". Exiting.");
allArgsValid = false;
// Verify the length parameter
} else if (args[i].equals("-length")) {
// If length is the only parameter (-length or -length 5 only is
// bad)
if (i == 0 && (i + 1 == args.length || i + 2 == args.length)) {
System.out.println("Length cannot be the only parameter. Exiting.");
allArgsValid = false;
// If what comes after -length is 0 or not a number
} else if (!args[i + 1].m
- Length
- Upper case letters
- Lower case letters
- Special characters
- Numbers
The project is on BitBucket.
Example program execution :
java -jar program.jar -length 12 -number -upper -lower -specialThis generates a 12 character long password with upper and lower case letters, numbers, and special characters.
What I think :
- I find my Parser class to be quite messy. It would be hard to add a new parameter with different properties. How would you do it?
Parser.java
```
package passManager;
import java.util.ArrayList;
import java.util.Arrays;
public class Parser {
public static ArrayList makeArgList(String args[]) {
ArrayList argList = new ArrayList();
boolean allArgsValid = true;
// Checks if each arg is valid
for (int i = 0; i < args.length && allArgsValid; i++) {
// If it starts with - and is not in the list of parameters, bad
if (args[i].startsWith("-") && !isInArgumentsList(args[i], Constants.VALID_ARGS)) {
System.out.println("Invalid parameter: " + args[i] + ". Exiting.");
allArgsValid = false;
// If it's not a parameter and it isn't a number, bad
} else if (!isInArgumentsList(args[i], Constants.VALID_ARGS) && !args[i].matches("^\\d+$")) {
System.out.println("Invalid parameter: " + args[i] + ". Exiting.");
allArgsValid = false;
// Verify the length parameter
} else if (args[i].equals("-length")) {
// If length is the only parameter (-length or -length 5 only is
// bad)
if (i == 0 && (i + 1 == args.length || i + 2 == args.length)) {
System.out.println("Length cannot be the only parameter. Exiting.");
allArgsValid = false;
// If what comes after -length is 0 or not a number
} else if (!args[i + 1].m
Solution
Command-line parsing
Instead of writing your bespoke methods to parse command-line options, consider using a library that can do that for you quite effortlessly. A quick Google search throws up the likes of Apache Commons CLI and JCommander.
Interfaces over implementations
You are passing around
Instead of writing your bespoke methods to parse command-line options, consider using a library that can do that for you quite effortlessly. A quick Google search throws up the likes of Apache Commons CLI and JCommander.
Interfaces over implementations
You are passing around
ArrayList as a method argument or return type, but all your methods need to know is just the List interface. Making this change is preferred as it allows you to swap different List implementations if required. E.g. if you are writing unit tests that can rely on Arrays.asList("some", "text") to give you a List, instead of having to create an ArrayList and then populate it.static fieldsGenerator.WORKING_SET is a static StringBuilder, but it is used in a decidedly non-static way. You are building a String to generate your characters from, so this String should reside within the method. Leaving it as a static field allows it to be abused quite easily, e.g. concurrent calls made to modify WORKING_SET with different acceptable values may not generate the right randomized results.Context
StackExchange Code Review Q#129478, answer score: 2
Revisions (0)
No revisions yet.