Simple password generator

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);

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 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 included;
    private HashSet excluded;

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

    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

StackExchange Code Review Q#111907, answer score: 10