HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Small password generator app with GUI

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
appwithgeneratorpasswordsmallgui

Problem

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:

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 resources/Locale.properties
*/
static ResourceBundle messages;
/**
* Loads GUI messages for locale specified in
* resources/Locale.properties from
* resources/MessagesBundle_<language>_<country>.properties
*/
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());

Solution

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.

Code Snippets

char[] pw = pg.generate(numberOfChars);
    for (int i = 0; i < pw.length; i++) {
        txtPW.append(Character.toString(pw[i]));
        pw[i] = 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);
}
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);
}
// Add empty space
    this.add(Box.createRigidArea(new Dimension(0, BORDER_WIDTH)));
public GUI() {
    setupPanel();

    setupNumberOfCharactersField();
    setupPasswordField();

    setupGeneratePasswordButton();
}

Context

StackExchange Code Review Q#66986, answer score: 10

Revisions (0)

No revisions yet.