4
\$\begingroup\$

I wrote the following password generator, to generate a password in which each character group appears at least at once.

import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

public class PWGen {
    public static final ArrayList<ArrayList<Character>> groups = new ArrayList<ArrayList<Character>>();
    static {
        ArrayList<Character> g1 = new ArrayList<Character>();
        for (int i = 'a'; i <= 'z'; i++) {
            g1.add((char) i);
        }
        // Remove similar characters
        for (Iterator<Character> iterator = g1.iterator(); iterator.hasNext();) {
            Character character = (Character) iterator.next();
            if (character == 'l')
                iterator.remove();
        }
        ArrayList<Character> g2 = new ArrayList<Character>();
        for (int i = 'A'; i <= 'Z'; i++) {
            g2.add((char) i);
        }
        // Remove similar characters
        for (Iterator<Character> iterator = g2.iterator(); iterator.hasNext();) {
            Character character = (Character) iterator.next();
            if ("IO".contains(String.valueOf(character)))
                iterator.remove();
        }
        ArrayList<Character> g3 = new ArrayList<Character>();
        for (int i = '0'; i <= '9'; i++) {
            g3.add((char) i);
        }
        ArrayList<Character> g4 = new ArrayList<Character>();
        g4.add('-');
        ArrayList<Character> g5 = new ArrayList<Character>();
        g5.add('_');
        ArrayList<Character> g6 = new ArrayList<Character>();
        g6.add(' ');
        ArrayList<Character> g7 = new ArrayList<Character>();
        g7.add('!');
        g7.add('$');
        g7.add('%');
        g7.add('&');
        groups.addAll(List.of(g1, g2, g3, g4, g5, g6, g7));
    }

    public static final String getPassword(final int len, final int gs, final boolean any) {
        SecureRandom sr = new SecureRandom();
        while (true) {
            StringBuilder b = new StringBuilder();
            int mask = 0;
            while (b.length() < len) {
                int i1 = sr.nextInt(7);
                if (((1 << i1) & (gs)) != 0) {
                    int i2 = sr.nextInt(groups.get(i1).size());
                    b.append(groups.get(i1).get(i2));
                    mask |= (1 << i1);
                }
            }
            if (!any || mask == gs) {
                return b.toString();
            }
        }
    }

    public static void main(String[] args) {
        System.out.println(getPassword(10, 0b1011111, false));
        System.out.println(getPassword(10, 0b1011111, true));
    }
}

Edit / Version 2

import java.security.SecureRandom;
import java.util.ArrayList;
import java.util.Iterator;
import java.util.List;

public class PWGen {
    public static final ArrayList<ArrayList<Character>> groups = new ArrayList<ArrayList<Character>>();
    static {
        ArrayList<Character> g1 = new ArrayList<Character>();
        for (int i = 'a'; i <= 'z'; i++) {
            g1.add((char) i);
        }
        // Remove similar characters
        for (Iterator<Character> iterator = g1.iterator(); iterator.hasNext();) {
            Character character = (Character) iterator.next();
            if (character == 'l')
                iterator.remove();
        }
        ArrayList<Character> g2 = new ArrayList<Character>();
        for (int i = 'A'; i <= 'Z'; i++) {
            g2.add((char) i);
        }
        // Remove similar characters
        for (Iterator<Character> iterator = g2.iterator(); iterator.hasNext();) {
            Character character = (Character) iterator.next();
            if ("IO".contains(String.valueOf(character)))
                iterator.remove();
        }
        ArrayList<Character> g3 = new ArrayList<Character>();
        for (int i = '0'; i <= '9'; i++) {
            g3.add((char) i);
        }
        ArrayList<Character> g4 = new ArrayList<Character>();
        g4.add('-');
        ArrayList<Character> g5 = new ArrayList<Character>();
        g5.add('_');
        ArrayList<Character> g6 = new ArrayList<Character>();
        g6.add(' ');
        ArrayList<Character> g7 = new ArrayList<Character>();
        g7.add('!');
        g7.add('$');
        g7.add('%');
        g7.add('&');
        groups.addAll(List.of(g1, g2, g3, g4, g5, g6, g7));
    }

    public static final String getPassword(final int len, final int gs, final boolean any) {
        ArrayList<Character> list = new ArrayList<Character>();
        for (int i = 0; i < 7; i++) {
            if (((1 << i) & gs) != 0) {
                for (Character character : groups.get(i)) {
                    list.add(character);
                    list.add((char) i);
                }
            }
        }
        int n = list.size() / 2;
        SecureRandom sr = new SecureRandom();
        while (true) {
            StringBuilder b = new StringBuilder();
            int mask = 0;
            while (b.length() < len) {
                int i1 = sr.nextInt(n) * 2;
                b.append(list.get(i1));
                mask |= (1 << list.get(i1 + 1));
            }
            if (!any || mask == gs) {
                return b.toString();
            }
        }
    }

    public static void main(String[] args) {
        System.out.println(getPassword(10, 0b1111111, false));
        System.out.println(getPassword(10, 0b1111111, true));
        System.out.println();
        System.out.println( getPassword(Integer.parseInt(args[0]), Integer.parseInt(args[1], 2), Boolean.parseBoolean(args[2])) );
        System.out.println();
    }
}

which version is better?

\$\endgroup\$
  • 2
    \$\begingroup\$ While "comparative reviews" (A vs B) are technically on-topic (see comparative-review), IMO you would get the best out of this site if you presented us the version of the code you're using, and expanded a bit more on how it's used; you can also include unit tests if you have any: give reviewers information about the intent of your code (readability/maintainability, performance, security, etc.), it helps defining what "better" means for you. Feel free to edit your post and tell us more! \$\endgroup\$ – Mathieu Guindon Oct 13 '19 at 2:21
2
\$\begingroup\$

To be honest: Neither.

  • Both are equally unnecessarily complicated and difficult to read. They use old-fashioned, maybe over-optimized techniques instead of Java/OOP features.

  • The code lacks readable variable names and any documentation for the reader/reviewer or for other programmers who need to use this.

  • And finally (if I read it correctly) they have the danger to getting caught in an infinite loop if called with specific arguments.


Building the character lists

Memory management

In the building of the character lists one of the problems is that the ArrayLists are used in a way that (internally) the data is copied in memory multiple times. To avoid that when filling an ArrayList, if you know number of elements you will be adding, set the capacity when creating the ArrayList:

ArrayList<Character> g1 = new ArrayList<Character>(26);

After filling the list you loop over the items to find one you remove. Removing items from an ArrayList also requires coping of data in order to fill the gaps in the array. Since you are already looping over the letters to fill the array list, you shouldn't add the letters you don't want in the first place:

ArrayList<Character> g1 = new ArrayList<Character>();
for (char i = 'a'; i <= 'z'; i++) {
    if (i != 'l') {
       g1.add(i);
    }
}

Also notice: 'a' is already a literal of the type char. Declaring i as a char avoids casting twice - one implicit and one explicit).

Use interfaces instead of specific classes

Don't declare object variables, generic types and method parameters as their specific type, but as the most constraining interface possible. So your character list should be:

public static final List<List<Character>> groups = new ArrayList<List<Character>>();

This is because code that uses this list does not need (and should not need) to know that these are ArrayLists.

This also allows you to use something other than ArrayList. For example the small lists of special characters would be much nicer the create with List.of (which uses its own implementation of List):

List<Character> g4 = List.of('-');
List<Character> g5 = List.of('_');
List<Character> g6 = List.of(' ');
List<Character> g7 = List.of('!', '$', '%', '&');

This also comes back to another memory management point. The line

 groups.addAll(List.of(g1, g2, g3, g4, g5, g6, g7));

creates a list (List.of) but its content is added to groups and then it's discarded again. Instead don't create an ArrayList and use the list created by List.of directly:

public static final List<List<Character>> groups;

static {
    // ...
    groups = List.of(g1, g2, g3, g4, g5, g6, g7);
}

A more Java appropriate data structure

(This review is getting quite a bit longer that I expected, so I'll be more abstract from here on.)

Is is really needed to fill an array with all possible characters? Instead consider a data structure that represents a character range for example storing only the first, last and missing characters. It can have methods that return the number of characters it represents and return the nth character.

Also the different character sets, which are currently represented by the index of the outer ArrayList and the bits in the gs parameter of the actual password generator, could be instead represented by a Java enum. Java also provides a EnumSet which would be used to replace the bitset.

(I could go into details and write much more, but unfortunately I don't have the time anymore.)

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ most constraining interface should be least constraining interface. The best practice in OOP is to pass around the weakest type that meets the requirements of the caller. \$\endgroup\$ – Reinderien Oct 14 '19 at 17:23
  • \$\begingroup\$ @Reinderien Maybe we have different definitions of "constrainting" in this context? For me, for example, Collection is more constrainting than List. \$\endgroup\$ – RoToRa Oct 15 '19 at 6:51

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.