8
\$\begingroup\$

I just wrote my first Java class, and I wanted to share it with you and maybe get some quick check from more experiences coders than I am.

I want to learn OOP, and Java is just the tool I thought was the best to start with (it's very similar to C in syntax, which I'm used to, and it's spreading more and more, day by day).

You can obviously comment on Java bad habits and ways to improve the class, but consider the class structure and the abstraction of the problem I wanted to solve, the separation between in data methods, etc.. as my main point of concern.

Here's the class. The aim is to create a random password generator.

import java.util.Random;

public final class PasswordGenerator {

  // DATAS
    // characters with which the password will be composed
    private static final int charactersSize = 100;
    private static char [] characters = new char [charactersSize];

    // keep the counts of used characters
    private static int charactersCount = 0;

    // size of the password to generate
    private int passwordSize;

  // CONSTRUCTOR
    public PasswordGenerator( int passwordSize ) {

      // set the password size
      this.passwordSize = passwordSize;

      // set the characters that will be used to generate the password
      initCharacters();
    }

  // METHODS
    // fill the array of characters that will be used to generate the password 
    private static char [] initCharacters() {
      int i = 0;

      // add 0-9
      for ( int j = 48; j < 58; ++i, ++j, ++charactersCount ) {
        characters[i] = (char) j;
      }

      // add @ + a-z
      for ( int j = 64; j < 91; ++i, ++j, ++charactersCount ) {
        characters[i] = (char) j;
      }

      // add A-Z
      for ( int j = 97; j < 123; ++i, ++j, ++charactersCount ) {
        characters[i] = (char) j;
      }

      return characters;
    }

    // generate a random password
    public char [] get() {

      // initialize the random number generator
      Random rnd = new Random();

      char [] password = new char [passwordSize];


      // choose a random character from the array
      for ( int i = 0; i < passwordSize; ++i ) {
        password[i] = characters[ rnd.nextInt(charactersCount) ];
      }

      return password;
    }

  // DEBUG METHODS
    // show the characters the will be used to compose the pass
    public void showCharacters() {
      for ( int i = 0; i < charactersCount && characters[i] != 0; ++i ) {
        System.out.println(characters[i]);
      }
    }

  // MAIN - testing code 
    public static void main(String[] args) {
      int passwordSize = 10;
      PasswordGenerator password = new PasswordGenerator( passwordSize );
      System.out.println( password.get() );   
    }

}
\$\endgroup\$
  • 3
    \$\begingroup\$ From a security perspective, I hope you're not going to use Random for real passwords. It produces low-quality random numbers. You should use SecureRandom instead for actual applications such as this. \$\endgroup\$ – user314104 Sep 28 '13 at 23:37
  • \$\begingroup\$ Related: stackoverflow.com/q/1741160/435605 \$\endgroup\$ – AlikElzin-kilaka May 26 '16 at 16:01
7
\$\begingroup\$

Your code does have its share of C-isms, but we will be able to fix that.

But first, if you are learning Java in order to understand Object Oriented Programming, pick a better language. Java's OO primitives are classes, abstract classes, interfaces and single inheritance. Since Java was designed, the world has moved on, and trait-based OO (instead of interfaces) is all the rage. The Scala language isn't very C-like, but has a wonderful object system. Another interesting language for OO is Ruby with it's metaclasses.

I assume you already know about the difference of static and non-static fields and methods. Whatever is marked as static belongs to the class and is shared among all objects of that class. Your fillCharacters static method only operates on the class level, yet you call it in an object constructor. Instead of refilling the characters each time you make a new generator, initialize that array once.

A horrible C-ism is that you make your characters array fixed size, in the hope that it will be large enough. But you only fill it with 63 characters! This is the way bugs are born. Java has multiple collections (like LinkedLists or ArrayLists) that resize themselves if necessary. I would do something like this:

private static char[] characters = initCharacters();

private static char[] initCharacters() {
  final int initialCapacity = 63;
  // a vector is a variable-size array
  final List<Character> chars = new Vector<Character>(initialCapacity);

  // add digits 0–9
  for (char c = '0'; c <= '9'; c++) {
    chars.add(c);
  }

  // add uppercase A–Z and '@'
  for (char c = '@'; c <= 'Z'; c++) {
    chars.add(c);
  }

  // add lowercase a–z
  for (char c = 'a'; c <= 'z'; c++) {
    chars.add(c);
  }

  // Copy the chars over to a simple array, now that we know
  // the length. The .toArray method could have been used here,
  // but its usage is a pain.

  final char[] charArray = new char[chars.size()];

  for (int i = 0; i < chars.size(); i++) {
    charArray[i] = chars.get(i).charValue();
  }

  return charArray;
}

private static void showCharacters() {
  System.err.println("The following " + characters.length + " characters are available: " + new String(characters));
}

What was that characters.length? Yes, Java keeps book about the size of your arrays, so you don't have to! Keeping extra variables around for the size is error prone. Also note that I allocate various objects in the initCharacters method – the garbage collector will clean up after me.

Because our character array has no unused fields, we don't need the charactersSize and charactersCount variables any longer.

Something that C doesn't have is generics, i.e. parameterized polymorphism. The arguments in angled brackets are type names, which can only be classes. Java has primitive types like char and int. These have corresponding object types like Character and Integer.

Your get method is mostly allright. I'd call it nextPassword, and would rename rnd to random. The algorithm is fine.


One thing I'd do differently would be not using much object orientation for this problem. Creating a new PasswordGenerator for different sizes is silly. I'd rather use that functionality like

System.out.println(PasswordGenerator.nextPassword(passwordSize));

i.e. use the generation as a static method, and pass the requested size as a parameter.

\$\endgroup\$
  • 3
    \$\begingroup\$ +1, nice answer. But have to disagree about Java being a bad language to learn object-oriented principles. I personally think that Scala "feels" very dirty and messy, but of course that's subjective. \$\endgroup\$ – asteri Sep 28 '13 at 17:19
  • 1
    \$\begingroup\$ Is there any reason you chose to use Vector rather than ArrayList? This topic would suggest you should avoid its use: stackoverflow.com/questions/1386275/… \$\endgroup\$ – Bort Oct 4 '13 at 21:58
3
\$\begingroup\$

Here's my take on this. This is actually port of my C# code which does the same, however, as Java doesn't support functional programming aspect of C#, it's a bit less verbose.

I'm using really simple Fluent interface here.

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Random;

public class PasswordBuilder {

private final static Random random = new Random();

// we keep our data in lists. Arrays would suffice as data never changes though.
private final static List<Character> LOWER_CAPS, UPPER_CAPS, DIGITS, SPECIALS;

// stores all templates
private List<Template> templateList = new ArrayList<Template>();

// indicates if we should shuffle the password
private boolean doShuffle;

/**
 * Factory method to create our builder.
 *
 * @return New PasswordBuilder instance.
 */
public static PasswordBuilder builder() {
    return new PasswordBuilder();
}

/**
 * Adds lowercase letters to password.
 *
 * @param count Number of lowercase letters to add.
 * @return This instance.
 */
public PasswordBuilder lowercase(int count) {
    templateList.add(new Template(LOWER_CAPS, count));
    return this;
}

public PasswordBuilder uppercase(int count) {
    templateList.add(new Template(UPPER_CAPS, count));
    return this;
}

public PasswordBuilder digits(int count) {
    templateList.add(new Template(DIGITS, count));
    return this;
}

public PasswordBuilder specials(int count) {
    templateList.add(new Template(SPECIALS, count));
    return this;
}

/**
 * Indicates that the password will be shuffled once
 * it's been generated.
 *
 * @return This instance.
 */
public PasswordBuilder shuffle() {
    doShuffle = true;
    return this;
}

/**
 * Builds the password.
 *
 * @return The password.
 */
public String build() {
    // we'll use StringBuilder
    StringBuilder passwordBuilder = new StringBuilder();
    List<Character> characters = new ArrayList<Character>();

    // we want just one list containing all the characters
    for (Template template : templateList) {
        characters.addAll(template.take());
    }

    // shuffle it if user wanted that
    if (doShuffle)
        Collections.shuffle(characters);

    // can't append List<Character> or Character[], so
    // we do it one at the time
    for (char chr : characters) {
        passwordBuilder.append(chr);
    }

    return passwordBuilder.toString();
}

// initialize statics
static {
    LOWER_CAPS = new ArrayList<Character>(26);
    UPPER_CAPS = new ArrayList<Character>(26);
    for (int i = 0; i < 26; i++) {
        LOWER_CAPS.add((char) (i + 'a'));
        UPPER_CAPS.add((char) (i + 'A'));
    }

    DIGITS = new ArrayList<Character>(10);
    for (int i = 0; i < 10; i++) {
        DIGITS.add((char) (i + '0'));
    }

    // add special characters. Note than other
    // than @, these are in ASCII range 33-43
    // so we could have used the loop as well
    SPECIALS = new ArrayList<Character>() {{
        add('!');
        add('@');
        add('#');
        add('$');
        add('%');
        add('^');
        add('&');
        add('(');
        add(')');
        add('*');
        add('+');
    }};
}
}

Here's the Template class. This class is used to tell us something like: From list of given characters, take n(count) characters by random.

import java.util.ArrayList;
import java.util.List;
import java.util.Random;

public class Template {
private final List<Character> source;
private final int             count;

private static final Random random = new Random();

public Template(List<Character> source, int count) {
    this.source = source;
    this.count = count;
}

public List<Character> take() {
    List<Character> taken = new ArrayList<Character>(count);
    for (int i = 0; i < count; i++) {
        taken.add(source.get(random.nextInt(source.size())));
    }

    return taken;
}
}

Last, but not the least, example usage:

PasswordBuilder builder = new PasswordBuilder();
    builder.lowercase(5)
            .uppercase(5)
            .specials(2)
            .digits(2)
            .shuffle();
    // write 100, 14-char shuffled passwords
    for (int i = 0; i < 100; i++) {
        System.out.println(builder.build());
    }

This example (hopefully) shows the power of OO. It was not necessary for this to be this complex, you can remove templates and just generate password on the fly, or you can improve them and add more features.

In the end, there are tens of different ways to implement this, but I hope I helped a bit. :)

\$\endgroup\$
  • \$\begingroup\$ +1, and I am not going to select this as the accepted answer just because due to my poor experience in the world of OOP, I can't get through your code so well as I can read amon's. I really thank you for the example, though. Indeed, the more I read it, the more it makes sense to me, and I already prefer your approach to the one I initially took. Thank you @Alex. \$\endgroup\$ – doplumi Oct 6 '13 at 18:12
  • 1
    \$\begingroup\$ why have you implemented a Fisher-Yates algorithm when there is a Collections.shuffle(List<?> list, Random rnd), available? \$\endgroup\$ – doplumi Oct 6 '13 at 21:29
  • \$\begingroup\$ check docs.oracle.com/javase/7/docs/api/java/util/… for that \$\endgroup\$ – doplumi Oct 6 '13 at 21:29
  • \$\begingroup\$ You should by all means accept the other answer, as It's based on your original code. I have just provided a way how would I myself would have implemented it, and of course I'm not claiming it's any better - moreover, it has its downsides. You are correct about Collections.shuffle(), I'll change the code to reflect that (No built-in shuffle in .net, so that was the reason as I hastly ported it ). \$\endgroup\$ – Alex Oct 7 '13 at 3:26
3
\$\begingroup\$

Its not required to write tons of lines and regex for this purpose now. We can use passey library that generates and validates passwords. Passwords can be alphanumeric or special characters.

For example following methods generates password with alphanumeric and special characters with a minimum length of 8 characters.

public String generateRandomPassword() {

        List rules = Arrays.asList(new CharacterRule(EnglishCharacterData.UpperCase, 1),
                new CharacterRule(EnglishCharacterData.LowerCase, 1), new CharacterRule(EnglishCharacterData.Digit, 1),new CharacterRule(EnglishCharacterData.Special, 1));

        PasswordGenerator generator = new PasswordGenerator();
        String password = generator.generatePassword(8, rules);
        return password;
    }

Reference: Generating random Password in Java

\$\endgroup\$
  • 1
    \$\begingroup\$ A bit more detail might be warranted, especially since this question has had an accepted answer for about three years. In any case, I don't think the author is still concerned about this code... \$\endgroup\$ – Graipher May 3 '17 at 8:19
  • \$\begingroup\$ I have updated the answer with an example \$\endgroup\$ – Dhiraj Ray May 3 '17 at 8:54
  • 1
    \$\begingroup\$ This may be helpful for others who are looking for the random password generator same as I was looking for it and found a better approach to generate it \$\endgroup\$ – Dhiraj Ray May 3 '17 at 9:01
  • 1
    \$\begingroup\$ That looks better already. I'm not familiar with the code formatting conventions in Java, but that list of character rules might need some nicer formatting. In addition you should also specifically add the requirements for this to work (passey, if I understand that link correctly). \$\endgroup\$ – Graipher May 3 '17 at 9:11
2
\$\begingroup\$

@doplumi, some feedback on you solution:

  1. It's not customizable. Hard to change the possible chars.
  2. You wrote the int value of the chars themselves, like 48. Hard to understand. Documentation can help but we should try to be more explicit in the code. Example: '0'.
  3. Many indexes in your loops. Hard to follow, especially if some error snuck in.

Implemented one myself as well. As a builder.

Supports password limit, must have characters and how much of them, char ranges and a list of them.

Usage Example:

System.out.println(new PasswordBuilder().addCharsOption("!@#$%&*()_-+=[]{}\\|:/?.,><", 1).addRangeOption('A', 'Z', 1).addRangeOption('a', 'z', 0).addRangeOption('0', '9', 1).build());

Example result: QU1GY7p+j+-PUW+_

System.out.println(new PasswordBuilder().addCharsOption("!@#$%&*()_-+=[]{}\\|:/?.,><", 1).addRangeOption('A', 'Z', 1).addRangeOption('a', 'z', 0).addRangeOption('0', '9', 1).setSize(5).build());

Example result: %,4NX

Implementation:

//Version=1.0
//Source=https://www.dropbox.com/s/3a4uyrd2kcqdo28/PasswordBuilder.java?dl=0
//Dependencies=java:7 com.google.guava:guava:18.0 commons-lang:commons-lang:2.6

import com.google.common.primitives.Chars;
import org.apache.commons.lang.ArrayUtils;

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

/**
 * Created by alik on 5/26/16.
 */
public class PasswordBuilder {
    private int size = 16;
    private List<Character> options = new ArrayList<>();
    private Map<List<Character>, Integer> musts = new java.util.LinkedHashMap<>();
    private SecureRandom secureRandom = new SecureRandom();

    public PasswordBuilder() {
    }

    public PasswordBuilder setSize(int size) {
        this.size = size;
        return this;
    }

    public PasswordBuilder addRangeOption(char from, char to, int mustCount) {
        List<Character> option = new ArrayList<>(to - from + 1);
        for (char i = from; i < to; ++i) {
            option.add(i);
        }
        return addOption(option, mustCount);
    }

    public PasswordBuilder addCharsOption(String chars, int mustCount) {
            return addOption(Chars.asList(chars.toCharArray()), mustCount);
    }

    public PasswordBuilder addOption(List<Character> option, int mustCount) {
        this.options.addAll(option);
        musts.put(option, mustCount);
        return this;

    }

    public String build() {
        validateMustsNotOverflowsSize();
        Character[] password = new Character[size];

        // Generate random from musts
        for (Map.Entry<List<Character>, Integer> entry : musts.entrySet()) {
            for (int i = 0; i < entry.getValue(); i++) {
                int charIndex = secureRandom.nextInt(entry.getKey().size());
                char c = entry.getKey().get(charIndex);
                addChar(password, c);
            }
        }

        // Generate from overall
        for (int i = 0; i < password.length; i++) {
            if (password[i] != null) continue;
            password[i] = options.get(secureRandom.nextInt(options.size()));
        }
        return new String(ArrayUtils.toPrimitive(password));
    }

    private void addChar(Character[] password, char c) {
        int i;
        for (i = secureRandom.nextInt(password.length); password[i] != null; i = secureRandom.nextInt(password.length)) {
        }
        password[i] = c;
    }

    private void validateMustsNotOverflowsSize() {
        int overallMusts = 0;
        for (Integer mustCount : musts.values()) {
            overallMusts += mustCount;
        }
        if (overallMusts > size) {
            throw new RuntimeException("Overall musts exceeds the requested size of the password.");
        }
    }

    public static void main(String[] args) {
        System.out.println(new PasswordBuilder().addCharsOption("!@#$%&*()_-+=[]{}\\|:/?.,><", 1).addRangeOption('A', 'Z', 1).addRangeOption('a', 'z', 0).addRangeOption('0', '9', 1).build());
        System.out.println(new PasswordBuilder().addCharsOption("!@#$%&*()_-+=[]{}\\|:/?.,><", 1).addRangeOption('A', 'Z', 1).addRangeOption('a', 'z', 0).addRangeOption('0', '9', 1).setSize(5).build());
    }
}
\$\endgroup\$

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.