Small password generator app with GUI

Question

This is a GUI-based random password generator, which supports internationalization.

It consists of 6 small classes:

Main classes:

• Application - entry point
• GUI - self-explanatory
• PassGen - class responsible for generating the password
• ExceptionReporter - utility class for displaying the critical error messages to the user

Test classes:

• MessagesBundleTest - tests internationalization resource bundles for completeness
• PassGenTest - ensures that alphabet used for password generation is correct

Plus internationalization resource bundles which are not included here.

Please review on generally accepted rules / conventions / best practices and how can it be improved.

This is what the app looks like on Windows:

This is how the app looks like on Windows

Application.java

package com.singularityfx.passgen;
import java.awt.EventQueue;
import java.io.IOException;
import java.io.InputStream;
import java.util.Locale;
import java.util.Properties;
import java.util.ResourceBundle;
import java.util.logging.Logger;
import javax.swing.UIManager;
import javax.swing.UIManager.LookAndFeelInfo;
import com.singularityfx.utils.ExceptionReporter;
/**
 * Application entry point.
 * Responsible for configuration of locale and look & feel, and for loading
 * the GUI.
 * @author SingularityFX
 *
 */
public class Application {
 private static Logger log = Logger.getLogger(Application.class.getName());
 /**
 * GUI messages for locale specified in <code>resources/Locale.properties</code>
 */
 static ResourceBundle messages;
 /**
 * Loads GUI messages for locale specified in 
 * <code>resources/Locale.properties</code> from 
 * <code>resources/MessagesBundle_&lt;language&gt;_&lt;country&gt;.properties</code>
 */
 private static void loadMessages() {
 Properties localeProp = new Properties();
 InputStream in = Application.class.getResourceAsStream("/Locale.properties");
 try {
 localeProp.load(in);
 } catch (IOException e) {
 log.severe(e.toString());
 new ExceptionReporter(e, true);
 }
 Locale locale = new Locale(
 localeProp.getProperty("language", "en"),
 localeProp.getProperty("country", "US"));
 messages = ResourceBundle.getBundle("MessagesBundle", locale);
 }
 /**
 * Creates Event Dispatch Thread and loads GUI
 */
 private static void loadGUI() {
 EventQueue.invokeLater(new Runnable() {
 public void run() {
 new GUI().createAndShowGUI();
 }
 });
 }
 /**
 * Checks if Nimbus look & feel is available. If yes - sets Nimbus as 
 * current look & feel. Otherwise default look & feel is used.
 */
 private static void setLookAndFeel() {
 try {
 for (LookAndFeelInfo info : UIManager.getInstalledLookAndFeels()) {
 if ("Nimbus".equals(info.getName())) {
 UIManager.setLookAndFeel(info.getClassName());
 break;
 }
 }
 } catch (Exception e) {
 StringBuilder msg = new StringBuilder();
 msg.append(e.toString()).append(". ")
 .append("Default look & feel will be used");
 log.info(msg.toString());
 }
 }
 /**
 * Entry point.
 */
 public static void main(String[] args) {
 loadMessages();
 setLookAndFeel();
 loadGUI(); 
 }
}

GUI.java

package com.singularityfx.passgen;
import java.awt.Dimension;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.BorderFactory;
import javax.swing.Box;
import javax.swing.BoxLayout;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextArea;
import javax.swing.JTextField;
import com.singularityfx.utils.ExceptionReporter;
/**
 * Represents Graphical User Interface. Creates and configures GUI components.
 * Includes ActionListener to launch password generation process.
 * @author SingularityFX
 */
@SuppressWarnings("serial")
public class GUI extends JPanel implements ActionListener {
 private static final int FRAME_MIN_WIDTH = 300;
 private static final int FRAME_MIN_HEIGHT = 0;
 private final int BORDER_WIDTH = 5;
 private JLabel lblNumberOfChars = 
 new JLabel(Application.messages.getString("lblNumberOfChars"));
 /**
 * Text field used to indicate number of characters in the generated password 
 */
 private JTextField txtNumberOfChars = new JTextField("8");
 private JLabel lblPW =
 new JLabel(Application.messages.getString("lblPW"));
 /**
 * Text area where generated password is displayed
 */
 private JTextArea txtPW = new JTextArea();
 /**
 * Button launching password generation
 */
 private JButton btnGenerate = 
 new JButton(Application.messages.getString("btnGenerate"));
 public GUI() {
 // Set up panel
 this.setLayout(new BoxLayout(this, BoxLayout.PAGE_AXIS));
 this.setBorder(BorderFactory.createEmptyBorder(
 BORDER_WIDTH, BORDER_WIDTH, BORDER_WIDTH, BORDER_WIDTH));
 // Set up lblNumberOfChars and add it to panel 
 lblNumberOfChars.setAlignmentX(CENTER_ALIGNMENT);
 this.add(lblNumberOfChars);
 // Set up txtNumberOfChars and add it to panel 
 txtNumberOfChars.setAlignmentX(CENTER_ALIGNMENT);
 txtNumberOfChars.setHorizontalAlignment(JTextField.LEFT);
 this.add(txtNumberOfChars);
 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));
 // Set up lblPW and add it to panel
 lblPW.setAlignmentX(CENTER_ALIGNMENT);
 this.add(lblPW);
 // Set up txtPW and add it to panel
 txtPW.setAlignmentX(CENTER_ALIGNMENT);
 this.add(txtPW);
 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));
 // Set up btnGenerate and add it to panel
 btnGenerate.setAlignmentX(CENTER_ALIGNMENT);
 btnGenerate.addActionListener(this);
 this.add(btnGenerate);
 }
 /**
 * Launches GUI.
 * Creates and configures JFrame, adds panel (represented by this class).
 */
 public void createAndShowGUI() {
 JFrame frame = new JFrame(Application.messages.getString("windowTitle"));
 frame.setMinimumSize(new Dimension(FRAME_MIN_WIDTH, FRAME_MIN_HEIGHT));
 frame.add(this);
 frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
 frame.pack();
 frame.setLocationRelativeTo(null);
 frame.setVisible(true);
 }
 /**
 * Listens to ActionEvent which is a signal to start password generation 
 * and displaying. Calls {@link #displayPw()} to do the actual job.
 * @see #displayPw()
 */
 @Override
 public void actionPerformed(ActionEvent evt) {
 if (evt.getSource() == btnGenerate) {
 displayPw();
 }
 }
 /**
 * Generates the password using {@link PassGen} and displays it in the GUI.
 */
 private void displayPw() {
 txtPW.setText("");
 PassGen pg = new PassGen();
 int numberOfChars = 0;
 try {
 numberOfChars = Integer.parseInt(txtNumberOfChars.getText());
 } catch (NumberFormatException ex) {
 new ExceptionReporter(null, false, 
 Application.messages.getString("numberFormatException"));
 }
 char[] pw = pg.generate(numberOfChars);
 for (int i = 0; i < pw.length; i++) {
 txtPW.append(Character.toString(pw[i]));
 pw[i] = 0;
 }
 }
}

PassGen.java

package com.singularityfx.passgen;
import java.util.ArrayList;
import java.util.List;
import java.util.Random;
/**
 * Pseudo-random password generator.
 * Generated password can be of any desired length and will consist of
 * alphanumerical characters only.
 * @author SingularityFX
 *
 */
public class PassGen {
 /**
 * List of characters that can be used in generated passwords 
 */
 private List<Character> alphabet = new ArrayList<Character>();
 private Random random = new Random();
 /**
 * Start index for ASCII numerical symbols range
 */
 private int asciiNumbersStart = 48;
 /**
 * End index for ASCII numerical symbols range
 */
 private int asciiNumbersEnd = 57;
 /**
 * Start index for ASCII lowercase latin alphabetical symbols range
 */
 private int asciiLowercaseStart = 97;
 /**
 * End index for ASCII lowercase latin alphabetical symbols range
 */
 private int asciiLowercaseEnd = 122;
 /**
 * Start index for ASCII uppercase latin alphabetical symbols range
 */
 private int asciiUppercaseStart = 65;
 /**
 * End index for ASCII uppercase latin alphabetical symbols range
 */
 private int asciiUppercaseEnd = 90;
 private int defaultPasswordLength = 12;
 /**
 * Sole constructor. Calls {@link #fillAlphabet(int, int)} for all
 * applicable ASCII ranges.
 * @see #fillAlphabet(int, int)
 */
 public PassGen() {
 fillAlphabet(asciiNumbersStart, asciiNumbersEnd);
 fillAlphabet(asciiLowercaseStart, asciiLowercaseEnd);
 fillAlphabet(asciiUppercaseStart, asciiUppercaseEnd);
 }
 /**
 * Adds the specified range of ASCII table to the alphabet that can be
 * used in password generation (including both startIndex and endIndex)
 * @param startIndex start index of the range (inclusive)
 * @param endIndex end index of the range (inclusive)
 */
 private void fillAlphabet(int startIndex, int endIndex) {
 for (int i = startIndex; i <= endIndex; i++) {
 alphabet.add((char)i);
 }
 }
 List<Character> getAlphabet() { // package access for unit testing purposes
 return this.alphabet;
 }
 /**
 * Generates the password of default length from the characters contained
 * in {@link #alphabet}.
 * @return generated password of default length
 */
 public char[] generate() {
 return generate(defaultPasswordLength);
 }
 /**
 * Generates the password of specified length from the characters contained
 * in {@link #alphabet}.
 * @param length length of password to be generated
 * @return generated password of specified length
 */
 public char[] generate(int length) {
 char[] pw = new char[length];
 for (int i = 0; i < length; i++) {
 pw[i] = alphabet.get(random.nextInt(alphabet.size()));
 }
 return pw;
 }
}

ExceptionReporter.java

package com.singularityfx.utils;
import javax.swing.JOptionPane;
/**
 * Utility class used to display unhandled exceptions.
 * Displays unhandled exception to user in a dialog window.
 * Shall be invoked from the <code>catch</code> clause
 * @author SingularityFX
 *
 */
public class ExceptionReporter {
 /**
 * Exception to be reported
 */
 private Exception ex;
 /**
 * Flag indicating if application shall be closed after exception is reported
 */
 private boolean closeApp;
 /**
 * Optional custom error message to be displayed to user
 */
 private String customMessage;
 /**
 * Platform-specific line separator
 */
 private String ls = System.getProperty("line.separator");
 /**
 * Constructor used when custom error message is not required
 * @param ex {@link #ex}
 * @param closeApp {@link #closeApp}
 */
 public ExceptionReporter(Exception ex, boolean closeApp) {
 this.ex = ex;
 this.closeApp = closeApp;
 report();
 }
 /**
 * Constructor used when custom error message is required
 * @param ex {@link #ex}
 * @param closeApp {@link #closeApp}
 * @param customMessage {@link #customMessage}
 */
 public ExceptionReporter(Exception ex, boolean closeApp, String customMessage) {
 this.ex = ex;
 this.closeApp = closeApp;
 this.customMessage = customMessage;
 report();
 }
 /**
 * Reports exception, then closes the application if necessary 
 * Exception is reported (as Exception.toString()) with additional custom 
 * error (if supplied during construction of ExceptionReporter object) to 
 * the user via dialog window.
 * Closes the application if {@link #closeApp} is <code>true</true>
 */
 private void report() {
 StringBuilder message = new StringBuilder();
 if (customMessage != null) { 
 message.append(customMessage)
 .append(ls)
 .append(ls);
 }
 if (ex != null) {
 message.append(ex.toString());
 }
 JOptionPane.showMessageDialog(null, message.toString(),
 "Error", JOptionPane.ERROR_MESSAGE);
 if (closeApp) {
 System.exit(1);
 }
 }
}

MessagesBundleTest.java

package com.singularityfx.passgen;
import java.util.Arrays;
import java.util.Collection;
import java.util.Locale;
import java.util.ResourceBundle;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
/**
 * Checks if messages bundles for all supported locales contains all 
 * required messages.
 * @author SingularityFX
 *
 */
@RunWith(Parameterized.class)
public class MessagesBundleTest {
 /**
 * List of required messages
 */
 private String[] messageKeys = {
 "windowTitle",
 "lblNumberOfChars",
 "lblPW",
 "btnGenerate",
 "numberFormatException",
 };
 private ResourceBundle messages;
 public MessagesBundleTest(String language, String country) {
 Locale locale = new Locale(language, country);
 this.messages = ResourceBundle.getBundle("MessagesBundle", locale); 
 }
 /**
 * @return representation of supported locales
 */
 @SuppressWarnings("rawtypes")
 @Parameterized.Parameters
 public static Collection locales() {
 return Arrays.asList(new Object[][] {
 {"en", "US"},
 {"ru", "RU"} 
 });
 }
 /**
 * Will fail with MissingResourceException if any of the message keys
 * are missing in the messages bundle.
 */
 @Test
 public void testMessagesAvailable() {
 for (String messageKey : messageKeys) {
 messages.getString(messageKey);
 }
 }
}

PassGenTest.java

package com.singularityfx.passgen;
import static org.junit.Assert.*;
import java.util.ArrayList;
import java.util.List;
import org.junit.BeforeClass;
import org.junit.Test;
/**
 * Compares the actual alphabet used for password generation, which is
 * constructed by specifying the appropriate ASCII ranges (referred to below as
 * "actual alphabet") to alphabet constructed from Strings (referred to below
 * as "String alphabet").
 * <p>
 * testAlphabetWantedIn() and testAlphabetUnwantedOut() together ensures the 
 * equality between "actual alphabet" and "String alphabet". 
 * @author SingularityFX
 *
 */
public class PassGenTest {
 /**
 * Password generator instance
 */
 private PassGen pg = new PassGen();
 /**
 * "String alphabet"
 * @see Class
 */
 private static List<Character> alphabet = new ArrayList<Character>();
 /**
 * Constructs the "String alphabet"
 * @see Class
 */
 @BeforeClass
 public static void setUp() {
 String alphabetString = "abcdefghijklmnopqrstuvwxyz";
 alphabetString += alphabetString.toUpperCase();
 alphabetString += "0123456789";
 char[] alphabetChars = alphabetString.toCharArray();
 for (int i = 0; i < alphabetChars.length; i++) {
 alphabet.add(alphabetChars[i]);
 }
 }
 /**
 * Ensures that all characters from "String alphabet" are inside 
 * "actual alphabet".
 * @see Class
 */
 @Test
 public void testAlphabetWantedIn() {
 for (Character c : alphabet) {
 assertTrue(pg.getAlphabet().contains(c));
 }
 }
 /**
 * Ensures that all characters from "actual alphabet" are inside
 * "String alphabet"
 * @see Class
 */
 @Test
 public void testAlphabetUnwantedOut() {
 for (Character c : pg.getAlphabet()) {
 assertTrue(alphabet.contains(c));
 }
 }
}

• Yann: Not really worth an answer on it's own, but since it's password generation, maybe a better Random?
• Grim: Please do not generate 1 and 0 because old people have learned to write I and O instead ( upload.wikimedia.org/wikipedia/commons/6/65/… ).
• njzk2: that's a lot of magic numbers. Consider at least (int) '0', or better even, "0123456789"
• maaartinus: @Yann4 I found it worth an answer. And the linked question's accepted answer is a non-sense security-wise, especially without https.
• Display Name: Why reinventing a wheel? There's Keychain Access, for example.

Answer 1

Your passGen class does not need to be that complicated even if you don't want to use Strings.

Your starting and ending values do not need to be changed for each class so they should be final. They will also not need to be different over all classes so static would be a good idea here (remember to use Capitals and underscores, the convention for constants).

private static final int ASCII_NUMBERS_START = 48;

However this is not a very clean way to generate a random alphanumeric String or char[].

A better way to do this whilst keeping the ability to customize what type of characters you would like to use would be to use Strings of each character type.

// Uppercase characters
private static final String UPPERCASE = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
// Lowercase characters
private static final String LOWERCASE = "abcdefghijklmnopqrstuvwxyz";
// Numeric characters
private static final String NUMERIC = "1234567890";

Then you would want a char[] containing all those characters so add each type together that is needed, using toCharArray() to turn it into a char[].

String characterString = "";
if (useUpperCase) characterString += UPPERCASE;
if (useLowerCase) characterString += LOWERCASE;
if (useNumeric) characterString += NUMERIC;
char[] characters = characterString.toCharArray();

Then go through the length of the password picking out a character randomly each time. As Random can be created each time the function is called this method can be static.

public static char[] getNewPassword(int length, boolean useUpperCase, boolean useLowerCase, boolean useNumeric) {
 String characterString = "";
 if (useUpperCase) characterString += UPPERCASE;
 if (useLowerCase) characterString += LOWERCASE;
 if (useNumeric) characterString += NUMERIC;
 char[] characters = characterString.toCharArray();
 Random random = new Random();
 char[] password = new char[length];
 for (int i = 0; i < length; i++) {
 password[i] = characters[random.nextInt(characters.length)];
 }
 return password;
}

Thus removing the need for an instance of passGen.

• njzk2: I would add an optional seed for the Random creation to allow for identical password re-generation.
• njzk2: I also tend not to like ifs without {}, even on 1 line
• Funky: @njzk2 Well make an edit proposal?
• Jason Crosby: I disagree with making getNewPassword static. its not really a "utility" method and classes with state should avoid having static methods when possible.
• Jason Crosby: @TheKittyKat Its holding onto a List, Random and a bunch of int's. IMHO static methods should be reserved for simple utility classes that don't need anything to do their job.

Answer 2

 char[] pw = pg.generate(numberOfChars);
 for (int i = 0; i < pw.length; i++) {
 txtPW.append(Character.toString(pw[i]));
 pw[i] = 0;
 }

What the...

Have you seen the String(char[] value) constructor? There's also String.valueOf(char[] data). Those would make your loop irrelevant... unless there's a specific reason you're setting the pw array to 0?

---

public GUI() {
 // Set up panel
 this.setLayout(new BoxLayout(this, BoxLayout.PAGE_AXIS));
 this.setBorder(BorderFactory.createEmptyBorder(
 BORDER_WIDTH, BORDER_WIDTH, BORDER_WIDTH, BORDER_WIDTH));
 // Set up lblNumberOfChars and add it to panel 
 lblNumberOfChars.setAlignmentX(CENTER_ALIGNMENT);
 this.add(lblNumberOfChars);
 // Set up txtNumberOfChars and add it to panel 
 txtNumberOfChars.setAlignmentX(CENTER_ALIGNMENT);
 txtNumberOfChars.setHorizontalAlignment(JTextField.LEFT);
 this.add(txtNumberOfChars);
 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));
 // Set up lblPW and add it to panel
 lblPW.setAlignmentX(CENTER_ALIGNMENT);
 this.add(lblPW);
 // Set up txtPW and add it to panel
 txtPW.setAlignmentX(CENTER_ALIGNMENT);
 this.add(txtPW);
 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));
 // Set up btnGenerate and add it to panel
 btnGenerate.setAlignmentX(CENTER_ALIGNMENT);
 btnGenerate.addActionListener(this);
 this.add(btnGenerate);
}

This function could make use of some meaningful whitespace.

public GUI() {
 // Set up panel
 this.setLayout(new BoxLayout(this, BoxLayout.PAGE_AXIS));
 this.setBorder(BorderFactory.createEmptyBorder(
 BORDER_WIDTH, BORDER_WIDTH, BORDER_WIDTH, BORDER_WIDTH));
 // Set up lblNumberOfChars and add it to panel 
 lblNumberOfChars.setAlignmentX(CENTER_ALIGNMENT);
 this.add(lblNumberOfChars);
 // Set up txtNumberOfChars and add it to panel 
 txtNumberOfChars.setAlignmentX(CENTER_ALIGNMENT);
 txtNumberOfChars.setHorizontalAlignment(JTextField.LEFT);
 this.add(txtNumberOfChars);
 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));
 // Set up lblPW and add it to panel
 lblPW.setAlignmentX(CENTER_ALIGNMENT);
 this.add(lblPW);
 // Set up txtPW and add it to panel
 txtPW.setAlignmentX(CENTER_ALIGNMENT);
 this.add(txtPW);
 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));
 // Set up btnGenerate and add it to panel
 btnGenerate.setAlignmentX(CENTER_ALIGNMENT);
 btnGenerate.addActionListener(this);
 this.add(btnGenerate);
}

Next, transform comments into code if they're explaining what the code does.

Specifically, this bit:

 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));

That should be a function addEmptySpaceToGUI().

And the rest? Well, that could be functions too.

public GUI() {
 setupPanel();
 setupNumberOfCharactersField();
 setupPasswordField();
 setupGeneratePasswordButton();
}

setupPanel() handles

 // Set up panel
 this.setLayout(new BoxLayout(this, BoxLayout.PAGE_AXIS));
 this.setBorder(BorderFactory.createEmptyBorder(
 BORDER_WIDTH, BORDER_WIDTH, BORDER_WIDTH, BORDER_WIDTH));

setupNumberOfCharactersField() handles

 // Set up lblNumberOfChars and add it to panel 
 lblNumberOfChars.setAlignmentX(CENTER_ALIGNMENT);
 this.add(lblNumberOfChars);
 // Set up txtNumberOfChars and add it to panel 
 txtNumberOfChars.setAlignmentX(CENTER_ALIGNMENT);
 txtNumberOfChars.setHorizontalAlignment(JTextField.LEFT);
 this.add(txtNumberOfChars);
 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));

setupPasswordField() handles

 // Set up lblPW and add it to panel
 lblPW.setAlignmentX(CENTER_ALIGNMENT);
 this.add(lblPW);
 // Set up txtPW and add it to panel
 txtPW.setAlignmentX(CENTER_ALIGNMENT);
 this.add(txtPW);
 // Add empty space
 this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));

and setupGeneratePasswordButton() handles

 // Set up btnGenerate and add it to panel
 btnGenerate.setAlignmentX(CENTER_ALIGNMENT);
 btnGenerate.addActionListener(this);
 this.add(btnGenerate);

This way, you keep all the specifics out of your main functions. Maybe the names aren't even right, and it's more like

public GUI() {
 setupPanel();
 addNumberOfCharactersField();
 addPasswordField();
 addGeneratePasswordButton();
}

This instead. With add instead of setup we imply some ordering.

• ultranoise: Hi! Thanks and +1 for your useful comments! I'm aware about String(char[] value). The idea was to improve security by avoiding creation of String, which is immutable and cannot be destroyed by any means other than GC. I.e. until garbage is collected, String containing password will be available in memory. char[], on the other hand, can be wiped to 0 (which I do here), thus erasing the password information. Of course, the password is still available in JTextArea. So maybe the whole idea was stupid, as the "security improvement" is not so big, but code clarity loss is obvious.

Answer 3

As it hasn't got addressed

You mostly have used good names for your variables.

/**
 * Start index for ASCII numerical symbols range
 */
private int asciiNumbersStart = 48;
/**
 * End index for ASCII numerical symbols range
 */
private int asciiNumbersEnd = 57;
/**
 * Start index for ASCII lowercase latin alphabetical symbols range
 */ 

The names implies without doubt the meaning of the variables but the comments here are just noise which should be removed.

Comments should describe the why something is done, but not what is done.

You have

private int defaultPasswordLength = 12; 

but you are showing 8 in your GUI. You should be consistent on this.

As you aren't using the parameterless generate() method, you can just get rid of it.

Your ExceptionReporter constructors should be chained like

public ExceptionReporter(Exception ex, boolean closeApp) {
 this(ex, closeApp, null);
}
public ExceptionReporter(Exception ex, boolean closeApp, String customMessage) {
 this.ex = ex;
 this.closeApp = closeApp;
 this.customMessage = customMessage;
 report();
} 

It is better to avoid exceptions, as exception handling is expensive. In the GUI class it would be better, to check, inside the displayPw() method, if the textbox contains any non digit characters instead of "blindly" calling Integer.parseInt(). An avoided exception is the best error handling.

• user36: "It is better to avoid exceptions, as exception handling is expensive." yes that really matters in a GUI password generator.

Answer 4

There are many other answers already, but they seem to miss this important point:

private Random random = new Random();

NO! Use SecureRandom if you want the password to be worth anything.

---

Why? The numbers generated by Random are rather predictable. The seed is predictable and the inner state is just 48 bits. So you can't get a stronger password out of it.

SecureRandom extends Random, it's a bit slower, but this doesn't matter here. So there's no reason not to use it.

• Pimgd: Why? char limit
• maaartinus: @Pimgd What? What char limit? See my update.
• Pimgd: Comment character limit.
• maaartinus: @Pimgd I see. No, I really wanted to write an answer rather than a comment as this is possibly more important than the other stuff. The most beautiful program is of no use if it hands out your password to everyone. I planed to add some more remarks, but there's so much already....

Answer 5

A minor detail: A good random character also contains non-alphanumeric characters. On my default azerty keyboard, I can see between 40 and 45 other characters that you haven't included that can be used, not even including any combinations. Fortunately, after you made the changes recommended by TheFailure, this will be trivial to add, 2 lines at most.

For reference, these are the signs:

23&|é@"#'([§^è!ç{à})°-_^ ̈]$*ù% ́`£μ,?;.:/=+~<>\

In case it isn't obvious, all these symbols are safe to use in a password, provided the recipient system handles the password properly (i.e. immediately hash it, never store it directly into the database,...)

Assuming you're making the changes from TheFailure, here's how to add them. At first I thought it was an easy addition, but apparently, Java doesn't have verbatim strings, so it gets somewhat trickier:

Add this line to the place where you declare the other character strings:

// Symbols 
private static final String SYMBOLS = "\u00b2\u00b3&|\u00e9@\"#\'([\u00a7^\u00e8!\u00e7{\u00e0})\u00b0-_^\u00a8]$*\u00f9%\u00b4`\u00a3\u00b5,?;.:/=+~<>\\";

Then at the place where you concatenated those other strings, add this:

if (useSymbols) characterString += SYMBOLS;

Please note that this is untested. I haven't used Java in years and this might not be the best method. I believe you can also use StringEscapeUtils.EscapeJava(), but that requires you to escape some characters in advance already.