Simple password generator

Question

Here is a simple password generator in Java:

public class PasswordGenerator {

    public static final String LOWER_CASE = "abcdefghijklmnopqrstuvwxyz";
    public static final String UPPER_CASE = LOWER_CASE.toUpperCase();
    public static final String DIGITS = "0123456789";
    public static final String PUNCTUATION_MARKS = "!,.:;@#$%^*()_-+={}\"<>?/\\№";
    public static final String SIMILAR_CHARACTERS = "Ll1ioO0";

    static class CharacterSet {
        private String include;
        private String exclude;

        public CharacterSet() {
            this.include = "";
            this.exclude = "";
        }

        public void include(String str) {
            include += str;
        }

        public void exclude(String str) {
            exclude += str;
        }

        public char randomCharacter(Random random) {
            int randomIndex;
            char randomCharacter;
            do {
                randomIndex = random.nextInt(include.length());
                randomCharacter = include.charAt(randomIndex);
            } while (exclude.contains(String.valueOf(randomCharacter)));
            return randomCharacter;
        }

    }

    public String generatePassword(PasswordSettings passwordSettings) {
        CharacterSet characterSet = characterSetFromSettings(passwordSettings);
        StringBuilder passwordBuilder = new StringBuilder();
        Random random = new Random(System.currentTimeMillis());
        for (int i = 0; i < passwordSettings.getLength(); ++i) {
            passwordBuilder.append(characterSet.randomCharacter(random));
        }
        return passwordBuilder.toString();
    }

    private CharacterSet characterSetFromSettings(PasswordSettings passwordSettings) {
        CharacterSet characterSet = new CharacterSet();
        if (passwordSettings.isLowerCase()) {
            characterSet.include(LOWER_CASE);
        }
        if (passwordSettings.isUpperCase()) {
            characterSet.include(UPPER_CASE);
        }
        if (passwordSettings.isDigits()) {
            characterSet.include(DIGITS);
        }
        if (passwordSettings.isPunctuationMarks()) {
            characterSet.include(PUNCTUATION_MARKS);
        }
        if (passwordSettings.isExcludeSimilarCharacters()) {
            characterSet.exclude(SIMILAR_CHARACTERS);
        }
        return characterSet;
    }


}

PasswordSettings is just a POJO class.

public class PasswordSettings implements Parcelable {

    public static final Parcelable.Creator<PasswordSettings> CREATOR = new Parcelable.Creator<PasswordSettings>() {
        @Override
        public PasswordSettings createFromParcel(Parcel source) {
            return new PasswordSettings(source);
        }

        @Override
        public PasswordSettings[] newArray(int size) {
            return new PasswordSettings[size];
        }
    };

    private int length;
    private boolean lowerCase;
    private boolean upperCase;
    private boolean digits;
    private boolean punctuationMarks;
    private boolean excludeSimilarCharacters;


    public int getLength() {
        return length;
    }

    public void setLength(int length) {
        this.length = length;
    }

    public boolean isLowerCase() {
        return lowerCase;
    }

    public void setLowerCase(boolean lowerCase) {
        this.lowerCase = lowerCase;
    }

    public boolean isUpperCase() {
        return upperCase;
    }

    public void setUpperCase(boolean upperCase) {
        this.upperCase = upperCase;
    }

    public boolean isDigits() {
        return digits;
    }

    public void setDigits(boolean digits) {
        this.digits = digits;
    }

    public boolean isPunctuationMarks() {
        return punctuationMarks;
    }

    public void setPunctuationMarks(boolean punctuationMarks) {
        this.punctuationMarks = punctuationMarks;
    }

    public boolean isExcludeSimilarCharacters() {
        return excludeSimilarCharacters;
    }

    public void setExcludeSimilarCharacters(boolean excludeSimilarCharacters) {
        this.excludeSimilarCharacters = excludeSimilarCharacters;
    }

    public PasswordSettings() {
        // empty constructor
    }

    public PasswordSettings(Parcel in) {
        length = in.readInt();
        lowerCase = ParcelHelper.readBoolean(in);
        upperCase = ParcelHelper.readBoolean(in);
        digits = ParcelHelper.readBoolean(in);
        punctuationMarks = ParcelHelper.readBoolean(in);
        excludeSimilarCharacters = ParcelHelper.readBoolean(in);
    }

    @Override
    public int describeContents() {
        return 0;
    }

    @Override
    public void writeToParcel(Parcel dest, int flags) {
        dest.writeInt(length);
        ParcelHelper.writeBoolean(dest, lowerCase);
        ParcelHelper.writeBoolean(dest, upperCase);
        ParcelHelper.writeBoolean(dest, digits);
        ParcelHelper.writeBoolean(dest, punctuationMarks);
        ParcelHelper.writeBoolean(dest, excludeSimilarCharacters);
    }

}

Answer 1

Keeping included and excluded characters, and picking a random character from the included set until you find one that's not in the excluded set is kinda ugly. If you think about it, in theory, it's possible that you will keep picking excluded elements forever... Sure, the chances of that maybe infinitesimally slim, but this is still ugly.

Alternatively, you could generate a character set from which the characters to exclude have already been excluded. Consider something like this:

public static class CharacterSet {
    private final String characters;
    private final Random random;

    private CharacterSet(String characters) {
        this.characters = characters;
        this.random = new Random();
    }

    public char getRandom() {
        return characters.charAt(random.nextInt(characters.length()));
    }

    public static class Builder { ... }
}

This CharacterSet class is a more clear and tight abstract data type:

• It contains a string of characters : naturally, hence the class name!
• It contains a getRandom() method to pick a random character
  • Notice that I dropped "character" from the method name: it was redundant, thanks to the class name
  • I also dropped the Random parameter, and moved it to a private field instead. This simplifies usage, with little to no drawbacks
• It contains a Builder, which can encapsulate the details of building up the character set from elements to include and exclude

Then the Builder can be implemented this way:

public static class Builder {
    private List<Character> included = new ArrayList<>();
    private Set<Character> excluded = new HashSet<>();

    public void include(String str) {
        for (char c : str.toCharArray()) {
            included.add(c);
        }
    }

    public void exclude(String str) {
        for (char c : str.toCharArray()) {
            excluded.add(c);
        }
    }

    public CharacterSet build() {
        StringBuilder builder = new StringBuilder(included.size());
        for (char c : included) {
            if (!excluded.contains(c)) {
                builder.append(c);
            }
        }
        return new CharacterSet(builder.toString());
    }
}

And the method that creates a CharacterSet from PasswordSettings can be rewritten in terms of the Builder like this:

private CharacterSet characterSetFromSettings(PasswordSettings passwordSettings) {
    CharacterSet.Builder builder = new CharacterSet.Builder();
    if (passwordSettings.isLowerCase()) {
        builder.include(LOWER_CASE);
    }
    if (passwordSettings.isUpperCase()) {
        builder.include(UPPER_CASE);
    }
    if (passwordSettings.isDigits()) {
        builder.include(DIGITS);
    }
    if (passwordSettings.isPunctuationMarks()) {
        builder.include(PUNCTUATION_MARKS);
    }
    if (passwordSettings.isExcludeSimilarCharacters()) {
        builder.exclude(SIMILAR_CHARACTERS);
    }
    return builder.build();
}

PasswordSettings

I'm wondering if you really need the setters. It would be great if you could remove them, and the parameterless constructor, and make the class immutable.

Answer 2

Your CharacterSet method could be more performant! Let's work on this first.

Using String to keep characters to include and exclude isn't a good plan. String concatenation is kind of expensive and researching in a String is a \$O(n)\$ operation, meaning you need to go through each element of your string to find if a character is contained.

You should use a HashSet<char> for excluded characters. The HashSet is a collection where each element is unique (which is another good thing for your set) and checking if an element is contained is an \$O(1)­\$ operation, because the elements are indexed by hashCode.

For your inserted characters, an ArrayList would do just fine.

This will be much faster than using two strings to keep your content.

Your variable names should reflect names, not verbs. Meaning include should be included.

public final class CharacterSet {
    private ArrayList<Character> included;
    private HashSet<Character> excluded;

    public CharacterSet() {
        this.included = new ArrayList<Character>();
        this.excluded = new HashSet<Character>();
    }

    public void include(Character... toInclude) {
        included.addAll(Arrays.asList(toInclude));
    }

    public void exclude(Character... toExclude) {
        for(char c : toExclude) {
            excluded.add(c);    
        }
    }

    public char getRandomCharacter(Random random) {
        char randomCharacter;

        do {
            int randomIndex = random.nextInt(included.size());
            randomCharacter = included.get(randomIndex);
        } while (excluded.contains(randomCharacter));

        return randomCharacter;
    }
}

Now, there's still a problem. Since your included list doesn't check for unique elements, your "randomness" is flawed. If I add the character a 100 times in 110 elements, there are much more chances for me to get an a then something else. But that's up to you and this issue wasn't addressed in your original code since your class isn't public, this isn't really a concern. It is fixed in the excluded set since sets contain unique element. We couldn't do the same thing with included since the sets do not have no mean to get an element at an index, which is needed for you.

Methods should have names that are verbs, not names. So randomCharacter should be getRandomCharacter.

Overall, you'll gain performance using these data structures over String.

The rest looks pretty good overall. You might want to give a little more empty lines in your code since multiple lines stuck together are hard to read. Also, don't forget to check your entry parameters for null in your public methods. Each time a NullPointerException is thrown because of a missed check, Santa removes a gift from your presents list. :p

Answer 3

I would recommend using java.security.SecureRandom (CSPRNG) instead. The default java.util.Random might not be secure enough, especially since you are using the current time as a seed. In any case, it is good practice to use SecureRandom whenever anything remotely resembling security (unpredictability of output by a malicious actor) is required.

On a related note, you need to be careful when you seed a PRNG by taking the current time - what happens when someone calls your function twice in quick succession? If they happen to both land on the same millisecond, you provide the same "random" output twice! One alternative is to seed once at startup and then reuse the existing instance. Using a CSPRNG usually avoids this with a better seed, typically provided by OS crypto APIs.