2
\$\begingroup\$

I made a random password generator in Java using a GUI. In the program, the user can choose the length, and whether to include lowercase letters, uppercase letters, symbols or numbers in the password. I believe that my code could be much better and I want to know how I can make it so.

Full code:

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.Random;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JSlider;
import javax.swing.JTextField;
import javax.swing.JCheckBox;
import java.awt.Color;

@SuppressWarnings("serial")
public class PasswordGenerator extends JFrame implements ActionListener {
    JSlider lengthChooser;
    JButton genButton;
    JTextField passField;
    JCheckBox lowercaseButton;
    JCheckBox uppercaseButton;
    JCheckBox symbolButton;
    JCheckBox numberButton;
    String lower = "abcdefghijklmnopqrstuvwxyz";
    String upper = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    String symbols = "~`!@#$%^&*()-_=+[{]}|;:'\",<>./?//";
    String numbers = "1234567890";
    private String password = "";

    public PasswordGenerator() {
        setTitle("Password Generator");
        getContentPane().setLayout(null);
        setSize(241, 262);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
        setVisible(true);
        setResizable(false);

        lengthChooser = new JSlider();
        getContentPane().add(lengthChooser);
        lengthChooser.setSnapToTicks(true);
        lengthChooser.setMaximum(30);
        lengthChooser.setToolTipText("Length of your password\r\n");
        lengthChooser.setBounds(10, 26, 200, 38);
        lengthChooser.setMajorTickSpacing(10);
        lengthChooser.setMinorTickSpacing(1);
        lengthChooser.setPaintTicks(true);
        lengthChooser.setPaintLabels(true);

        JLabel lblLength = new JLabel("Length:");
        getContentPane().add(lblLength);
        lblLength.setBounds(10, 11, 46, 14);

        genButton = new JButton("Generate");
        getContentPane().add(genButton);
        genButton.addActionListener(this);
        genButton.setBounds(71, 75, 89, 23);

        passField = new JTextField();
        getContentPane().add(passField);
        passField.setBounds(10, 184, 215, 38);

        JLabel lblPassword = new JLabel("Password:");
        getContentPane().add(lblPassword);
        lblPassword.setBounds(10, 159, 71, 14);

        lowercaseButton = new JCheckBox("Lowercase");
        getContentPane().add(lowercaseButton);
        lowercaseButton.setBounds(10, 105, 97, 23);

        uppercaseButton = new JCheckBox("Uppercase");
        getContentPane().add(uppercaseButton);
        uppercaseButton.setBounds(121, 105, 97, 23);

        symbolButton = new JCheckBox("Symbols");
        getContentPane().add(symbolButton);
        symbolButton.setBounds(10, 129, 97, 23);

        numberButton = new JCheckBox("Numbers");
        getContentPane().add(numberButton);
        numberButton.setBounds(121, 131, 97, 23);
    }

    String charString = "";

    @Override
    public void actionPerformed(ActionEvent event) {
        Random random = new Random();
        if (event.getSource() == genButton) {
            boolean boxesArentChecked = !numberButton.isSelected() && !symbolButton.isSelected() && !uppercaseButton.isSelected()
                    && !lowercaseButton.isSelected();
            boolean numIsZero = lengthChooser.getValue() == 0;
            if (boxesArentChecked && numIsZero) {
                passField.setForeground(Color.RED);
                passField.setText("Select a box and a size!");

            } else if(numIsZero) {
                passField.setForeground(Color.RED);
                passField.setText("Your password length is 0!");
            } else if(boxesArentChecked) {
                passField.setForeground(Color.RED);
                passField.setText("Select a box!");
            } else {
                passField.setForeground(Color.BLACK);
                if (lowercaseButton.isSelected()) {
                    charString += lower;
                }

                if (uppercaseButton.isSelected()) {
                    charString += upper;
                }

                if (symbolButton.isSelected()) {
                    charString += symbols;
                }

                if (numberButton.isSelected()) {
                    charString += numbers;
                }

                char[] characters = charString.toCharArray();

                password = "";
                for (int i = 1; i <= lengthChooser.getValue(); i++) {
                    int choice = random.nextInt(characters.length);
                    password += characters[choice];
                }
                charString = "";
                passField.setText(password);
            }
        }
    }
}

Constructor:

public PasswordGenerator() {
    setTitle("Password Generator");
    getContentPane().setLayout(null);
    setSize(241, 262);
    setDefaultCloseOperation(EXIT_ON_CLOSE);
    setVisible(true);
    setResizable(false);

    lengthChooser = new JSlider();
    getContentPane().add(lengthChooser);
    lengthChooser.setSnapToTicks(true);
    lengthChooser.setMaximum(30);
    lengthChooser.setToolTipText("Length of your password\r\n");
    lengthChooser.setBounds(10, 26, 200, 38);
    lengthChooser.setMajorTickSpacing(10);
    lengthChooser.setMinorTickSpacing(1);
    lengthChooser.setPaintTicks(true);
    lengthChooser.setPaintLabels(true);

    JLabel lblLength = new JLabel("Length:");
    getContentPane().add(lblLength);
    lblLength.setBounds(10, 11, 46, 14);

    genButton = new JButton("Generate");
    getContentPane().add(genButton);
    genButton.addActionListener(this);
    genButton.setBounds(71, 75, 89, 23);

    passField = new JTextField();
    getContentPane().add(passField);
    passField.setBounds(10, 184, 215, 38);

    JLabel lblPassword = new JLabel("Password:");
    getContentPane().add(lblPassword);
    lblPassword.setBounds(10, 159, 71, 14);

    lowercaseButton = new JCheckBox("Lowercase");
    getContentPane().add(lowercaseButton);
    lowercaseButton.setBounds(10, 105, 97, 23);

    uppercaseButton = new JCheckBox("Uppercase");
    getContentPane().add(uppercaseButton);
    uppercaseButton.setBounds(121, 105, 97, 23);

    symbolButton = new JCheckBox("Symbols");
    getContentPane().add(symbolButton);
    symbolButton.setBounds(10, 129, 97, 23);

    numberButton = new JCheckBox("Numbers");
    getContentPane().add(numberButton);
    numberButton.setBounds(121, 131, 97, 23);
}

ActionListener method:

@Override
    public void actionPerformed(ActionEvent event) {
        Random random = new Random();
        if (event.getSource() == genButton) {
            boolean boxesArentChecked = !numberButton.isSelected() && !symbolButton.isSelected() && !uppercaseButton.isSelected()
                    && !lowercaseButton.isSelected();
            boolean numIsZero = lengthChooser.getValue() == 0;
            if (boxesArentChecked && numIsZero) {
                passField.setForeground(Color.RED);
                passField.setText("Select a box and a size!");

            } else if(numIsZero) {
                passField.setForeground(Color.RED);
                passField.setText("Your password length is 0!");
            } else if(boxesArentChecked) {
                passField.setForeground(Color.RED);
                passField.setText("Select a box!");
            } else {
                passField.setForeground(Color.BLACK);
                if (lowercaseButton.isSelected()) {
                    charString += lower;
                }

                if (uppercaseButton.isSelected()) {
                    charString += upper;
                }

                if (symbolButton.isSelected()) {
                    charString += symbols;
                }

                if (numberButton.isSelected()) {
                    charString += numbers;
                }

                char[] characters = charString.toCharArray();

                password = "";
                for (int i = 1; i <= lengthChooser.getValue(); i++) {
                    int choice = random.nextInt(characters.length);
                    password += characters[choice];
                }
                charString = "";
                passField.setText(password);
            }
        }
    }
\$\endgroup\$

3 Answers 3

Reset to default
2
\$\begingroup\$

I concur with most of what the existing two answers say, but there are some other important issues which I think they missed.

Learn to use layout managers

Manual layout with setBounds is acceptable for an application which you are going to use once and then throw away, but it's hard work to lay things out correctly and it's extremely brittle. When you want to change a text, you have to recalculate everything. If you want to support localisation, you're really stuck.

AWT has various layout managers which will do a lot of that hard work automatically. I personally prefer GridBagLayout for most purposes, but it's worth spending time to get to know the options.

Don't let the user give you invalid input

When I see

            boolean numIsZero = lengthChooser.getValue() == 0;

the question I ask myself is "How can lengthChooser.getValue() ever be zero?" One call to setMinimum and the user is required to give me a sensible value.

That's not to say that you shouldn't have sanity checks, but they should be sanity checks rather than intentional interface elements.

Similarly, I would add some listeners to the checkboxes to enforce that at least one of lower case and upper case letters must be selected at all times.

\$\endgroup\$
2
\$\begingroup\$

For the future: Some indication of your skill level will help target our advice. I'm guessing you're still a beginner.

I'd start by extracting the password generation code into a separate class and passing it the relevant details to make it build the password requested. This will make it easier to reuse elsewhere (I know, this is a small app, but in bigger apps this sort of code reuse can be very handy).

When you're building a string, i.e. in 

password = "";
for (int i = 1; i <= lengthChooser.getValue(); i++) {
    int choice = random.nextInt(characters.length);
    password += characters[choice];
}

It would be more efficient to use the StringBuilder class...

StringBuilder passwordBuilder = new StringBuilder();
for (int i = 1; i <= lengthChooser.getValue(); i++) {
    int choice = random.nextInt(characters.length);
    password.append(characters[choice]);
}
password = passwordBuilder.toString();

Every time you concatenate two Strings with '+', Java instantiates a new StringBuilder in the background and uses that to join the Strings together (Very inefficient for cases like this!).

\$\endgroup\$
2
\$\begingroup\$

For anything related to security, prefer SecureRandom over the Random class. It has more randomness available.

The fields charString and password should be local variables inside the method where they are used.

The error message should not be written into the password field, but into a separate control, e.g. a label. Then you can set the color to red once and for all, and when there is no error, just make the label's text "", so that it is invisible.

Instead of repeating the setBounds call, you could define a method like this:

void placeAt(int left, int top, int width, int height, JControl control) {
  control.setBounds(left, top, width, height);
  getContentPane().add(control);
}

Using this method, you can shorten the code to:

placeAt(10, 50, 93, 25, new JLabel("Label text"));

(I chose to put the coordinates first in the parameter list, since they will be of similar length in the code, so that the JControl arguments almost align vertically. You could equally well choose to put the coordinates last, since the control is more important than the coordinates.)

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.