6
\$\begingroup\$

I have a small program in Java with GUI, which is generating passwords using Secure Random. I am posting my code here because I want to correct myself in style and, eventually, in efficiency of writing code.

Should I use try catch in, for example, button handling or password generating? I saw some user answers who discouraged using try catch statement. I am reading a lot, and I see that everyone has own opinion.

On Win7 & NetBeans IDE program look like this:

password Generator

I have 4 files:

Main.java

package passwordgenerator;

import java.awt.EventQueue;

public class Main {

    private static void loadGUI() {
            EventQueue.invokeLater(() -> {
                GUI gui = new GUI();
                gui.createAndShowGUI();
                gui.setVisible(true);
            });
    }

    public static void main(String[] args) {
        loadGUI();
    }
}

GUI.java (my functions are on bottom of this file) edited

package passwordgenerator;

import java.awt.Toolkit;
import java.awt.datatransfer.Clipboard;
import java.awt.datatransfer.StringSelection;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JSlider;

@SuppressWarnings("serial")
public class GUI extends JPanel implements ActionListener {

    private static final String UPPERCASE_CHARACTERS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    private static final String LOWERCASE_CHARACTERS = "abcdefghijklmnopqrstuvwxyz";
    private static final String NUMERIC_CHARACTERS = "1234567890";
    private static final String SPECIAL_CHARACTERS = "~!@#$%^&*()_|";

    /**
     *  Launches GUI.
     *  Creates and configures JFrame, adds panel (represented by this class).
     */
    public void createAndShowGUI() {
        initComponents();
        JFrame frame = new JFrame("password generator @re");
        //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);
    }

    /**
     * This method is called from within the constructor to initialize the form.
     * WARNING: Do NOT modify this code. The content of this method is always
     * regenerated by the Form Editor.
     */
    @SuppressWarnings("unchecked")
    // <editor-fold defaultstate="collapsed" desc="Generated Code">                          
    private void initComponents() {

        buttonGroup1 = new javax.swing.ButtonGroup();
        buttonGroup2 = new javax.swing.ButtonGroup();
        buttonGroup3 = new javax.swing.ButtonGroup();
        buttonGroup4 = new javax.swing.ButtonGroup();
        btnGenerate = new javax.swing.JButton();
        jButton2 = new javax.swing.JButton();
        jScrollPane1 = new javax.swing.JScrollPane();
        passwordField = new javax.swing.JTextArea();
        useAlphanumeric = new javax.swing.JCheckBox();
        useSpecialChars = new javax.swing.JCheckBox();
        jLabel1 = new javax.swing.JLabel();
        useLowerAlphabet = new javax.swing.JCheckBox();
        jSlider1 = new javax.swing.JSlider();
        jLabel2 = new javax.swing.JLabel();
        numberOfCharacters = new javax.swing.JLabel();
        jLabel4 = new javax.swing.JLabel();

        btnGenerate.setMnemonic('\r');
        btnGenerate.setText("generate password");
        btnGenerate.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(java.awt.event.ActionEvent evt) {
                btnGenerateActionPerformed(evt);
            }
        });

        jButton2.setText("copy to clipboard");
        jButton2.addActionListener(new java.awt.event.ActionListener() {
            public void actionPerformed(java.awt.event.ActionEvent evt) {
                copyToClipboard(evt);
            }
        });

        passwordField.setColumns(20);
        passwordField.setRows(5);
        jScrollPane1.setViewportView(passwordField);

        useAlphanumeric.setSelected(true);
        useAlphanumeric.setText("1-9");

        useSpecialChars.setSelected(true);
        useSpecialChars.setText("special chars (~!@#$...)");

        jLabel1.setText("mixing password chars ");

        useLowerAlphabet.setSelected(true);
        useLowerAlphabet.setText("a-z");

        jSlider1.setMajorTickSpacing(3);
        jSlider1.setMaximum(30);
        jSlider1.setMinimum(5);
        jSlider1.setMinorTickSpacing(1);
        jSlider1.setPaintLabels(true);
        jSlider1.setPaintTicks(true);
        jSlider1.setValue(8);
        jSlider1.addChangeListener(new javax.swing.event.ChangeListener() {
            public void stateChanged(javax.swing.event.ChangeEvent evt) {
                sliderStateChanged(evt);
            }
        });

        jLabel2.setText("number of characters:");

        numberOfCharacters.setText("8");

        jLabel4.setText("✓ A-Z");

        javax.swing.GroupLayout layout = new javax.swing.GroupLayout(this);
        this.setLayout(layout);
        layout.setHorizontalGroup(
            layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
            .addGroup(layout.createSequentialGroup()
                .addGap(26, 26, 26)
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                    .addGroup(layout.createSequentialGroup()
                        .addComponent(jScrollPane1, javax.swing.GroupLayout.PREFERRED_SIZE, 347, javax.swing.GroupLayout.PREFERRED_SIZE)
                        .addGap(0, 0, Short.MAX_VALUE))
                    .addGroup(layout.createSequentialGroup()
                        .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                            .addComponent(useSpecialChars)
                            .addComponent(useAlphanumeric))
                        .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                            .addGroup(layout.createSequentialGroup()
                                .addGap(62, 62, 62)
                                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                                    .addComponent(useLowerAlphabet)
                                    .addComponent(numberOfCharacters, javax.swing.GroupLayout.PREFERRED_SIZE, 30, javax.swing.GroupLayout.PREFERRED_SIZE))
                                .addContainerGap(javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE))
                            .addGroup(javax.swing.GroupLayout.Alignment.TRAILING, layout.createSequentialGroup()
                                .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED, javax.swing.GroupLayout.DEFAULT_SIZE, Short.MAX_VALUE)
                                .addComponent(jLabel4, javax.swing.GroupLayout.PREFERRED_SIZE, 41, javax.swing.GroupLayout.PREFERRED_SIZE)
                                .addGap(117, 117, 117))))))
            .addGroup(layout.createSequentialGroup()
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                    .addGroup(layout.createSequentialGroup()
                        .addGap(56, 56, 56)
                        .addComponent(jButton2)
                        .addGap(18, 18, 18)
                        .addComponent(btnGenerate, javax.swing.GroupLayout.PREFERRED_SIZE, 160, javax.swing.GroupLayout.PREFERRED_SIZE))
                    .addGroup(layout.createSequentialGroup()
                        .addGap(102, 102, 102)
                        .addComponent(jLabel2)))
                .addContainerGap(51, Short.MAX_VALUE))
            .addGroup(layout.createSequentialGroup()
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                    .addGroup(layout.createSequentialGroup()
                        .addGap(91, 91, 91)
                        .addComponent(jLabel1))
                    .addGroup(layout.createSequentialGroup()
                        .addGap(44, 44, 44)
                        .addComponent(jSlider1, javax.swing.GroupLayout.PREFERRED_SIZE, 313, javax.swing.GroupLayout.PREFERRED_SIZE)))
                .addGap(0, 0, Short.MAX_VALUE))
        );
        layout.setVerticalGroup(
            layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
            .addGroup(javax.swing.GroupLayout.Alignment.TRAILING, layout.createSequentialGroup()
                .addGap(14, 14, 14)
                .addComponent(jLabel1)
                .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.UNRELATED)
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                    .addComponent(useAlphanumeric)
                    .addComponent(jLabel4))
                .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.UNRELATED)
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                    .addComponent(useSpecialChars)
                    .addComponent(useLowerAlphabet))
                .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED, 3, Short.MAX_VALUE)
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.LEADING)
                    .addComponent(numberOfCharacters)
                    .addComponent(jLabel2))
                .addPreferredGap(javax.swing.LayoutStyle.ComponentPlacement.RELATED)
                .addComponent(jSlider1, javax.swing.GroupLayout.PREFERRED_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.PREFERRED_SIZE)
                .addGap(18, 18, 18)
                .addComponent(jScrollPane1, javax.swing.GroupLayout.PREFERRED_SIZE, javax.swing.GroupLayout.DEFAULT_SIZE, javax.swing.GroupLayout.PREFERRED_SIZE)
                .addGap(18, 18, 18)
                .addGroup(layout.createParallelGroup(javax.swing.GroupLayout.Alignment.BASELINE)
                    .addComponent(jButton2, javax.swing.GroupLayout.PREFERRED_SIZE, 32, javax.swing.GroupLayout.PREFERRED_SIZE)
                    .addComponent(btnGenerate, javax.swing.GroupLayout.PREFERRED_SIZE, 32, javax.swing.GroupLayout.PREFERRED_SIZE))
                .addGap(15, 15, 15))
        );
    }// </editor-fold>                        

    private void btnGenerateActionPerformed(java.awt.event.ActionEvent evt) {                                            
        String charactersToBeUsed = "";

        charactersToBeUsed = UPPERCASE_CHARACTERS; // always

        if (useLowerAlphabet.isSelected()) {
            charactersToBeUsed += LOWERCASE_CHARACTERS;
        }
        if (useAlphanumeric.isSelected()) {
            charactersToBeUsed += NUMERIC_CHARACTERS;
        }
        if (useSpecialChars.isSelected()) {
            charactersToBeUsed += SPECIAL_CHARACTERS;
        }
        PasswordGenerator pg = new PasswordGenerator(charactersToBeUsed);
        int numberOfChars = Integer.parseInt(numberOfCharacters.getText());

        String pw = pg.generate(numberOfChars);

        passwordField.setEditable(false);
        passwordField.setText(pw);
    }                                           

    private void sliderStateChanged(javax.swing.event.ChangeEvent evt) {                                    
        JSlider source = (JSlider) evt.getSource();
        if (!source.getValueIsAdjusting()) {
            numberOfCharacters.setText(Integer.toString(source.getValue()));
        }
    }                                   

    private void copyToClipboard(java.awt.event.ActionEvent evt) {                                 
        String myString = passwordField.getText();
        StringSelection stringSelection = new StringSelection (myString);
        Clipboard clpbrd = Toolkit.getDefaultToolkit().getSystemClipboard ();
        clpbrd.setContents (stringSelection, null);
    }                                


    // Variables declaration - do not modify                     
    private javax.swing.JButton btnGenerate;
    private javax.swing.ButtonGroup buttonGroup1;
    private javax.swing.ButtonGroup buttonGroup2;
    private javax.swing.ButtonGroup buttonGroup3;
    private javax.swing.ButtonGroup buttonGroup4;
    private javax.swing.JButton jButton2;
    private javax.swing.JLabel jLabel1;
    private javax.swing.JLabel jLabel2;
    private javax.swing.JLabel jLabel4;
    private javax.swing.JScrollPane jScrollPane1;
    private static javax.swing.JSlider jSlider1;
    private javax.swing.JLabel numberOfCharacters;
    public static javax.swing.JTextArea passwordField;
    public static javax.swing.JCheckBox useAlphanumeric;
    public static javax.swing.JCheckBox useLowerAlphabet;
    public static javax.swing.JCheckBox useSpecialChars;
    // End of variables declaration                   


    @Override
    public void actionPerformed(ActionEvent ae) {
        throw new UnsupportedOperationException("Not supported yet.");
    }
}

PasswordGenerator.java edited

(Based on this other post)

package passwordgenerator;

import java.security.SecureRandom;

public class PasswordGenerator {
    String charactersToBeUsed;

    private static final String UPPERCASE_CHARACTERS = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
    private static final int DEFAULT_PASSWORD_LENGTH = 8;

    // default constructor
    public PasswordGenerator() { 
        this(UPPERCASE_CHARACTERS);
    }

    // constructor
    public PasswordGenerator(String characters) {
        this.charactersToBeUsed = characters;
    }

    public String generate() {
        return generate(DEFAULT_PASSWORD_LENGTH);
    }

    public String generate(int length) {
        char[] password = new char[length];

        char[] possibleCharacters = charactersToBeUsed.toCharArray();
        SecureRandom r = new SecureRandom();

        for (int i = 0; i < length; i++) {
            password[i] = possibleCharacters[r.nextInt(possibleCharacters.length)];
        }
        return new String(password);
    }
}

EnumAsciiCharacters.java

package passwordgenerator;

public enum EnumAsciiCharacters {

    UPPERCASE( "ABCDEFGHIJKLMNOPQRSTUVWXYZ" ),
    LOWERCASE( "abcdefghijklmnopqrstuvwxyz" ),
    NUMERIC( "1234567890" ),
    SPECIAL( "~!@#$%^&*()_|" );

    private String characters;

    @Override
    public String toString() {
      return this.characters;
    }

    private EnumAsciiCharacters( String characters ) {
      this.characters = characters;
    }
} // EnumAsciiCharacters
Improve this question
\$\endgroup\$
1
  • 1
    \$\begingroup\$ There's a typo at password[i] += ... it should be password[i] = ... \$\endgroup\$
    – Gerold Broser
    Jun 9, 2015 at 12:24

2 Answers 2

Reset to default
2
\$\begingroup\$

1) I'd extend the names of UPPERCASE, LOWERCASE, NUMERIC, SPECIAL with _CHARACTERS. That's clearer when using them later.

2) Since defaultPasswordLength is a final I'd name it DEFAULT_PASSWORD_LENGTH.

3) characterString isn't a good name. It's obvious to any decent Java programmer that this is a String and a string contains characters. I'd name it charactersToBeUsed.

4) I'd not refer to GUI inside PasswordGenerator. Thus it cannot be used without a GUI - in a command line tool, for instance. And it can't even be used without a GUI of a special kind: the one you provide.

I'd use Dependency Injection (DI) instead. That means supplying the PasswordGenerator with the characters to be used from outside. That's much more flexible for possible future users (=developers) of your class. So, inject (String charactersToBeUsed) via a constructor or via the generate() methods. Define a default in case this argument isn't given.

Think of Separation of Concerns (SoC): It's not up to the password generator to decide (via finals) which (limited) characters can or should be used. It's being told to it from its user.

Take a radio as an example: It's not up to the radio to decide which (limited) set of channels it can play. You tell it which channels you want to listen to and it plays them then.

5) Instead of length*length*length*length*lengthI'd use java.Math.pow(length, 5).

6) I'd use an array instead of (relatively slow) String concatenation at the generation (and rename characters to possibleCharacters):

char[] password = new char[length];
...
char[] possibleCharacters = charactersToBeUsed.toCharArray();
...
for (int i = 0; i < length; i++) {
  password[i] = possibleCharacters[r.nextInt(possibleCharacters.length)];
}
return new String(password);

7) Make charactersToBeUsed private.

8) For the convenience of users of your PG you could supply an enum (with a constructor) containing your default characters:

package passwordgenerator;

public enum AsciiCharacters
  {
  UPPERCASE( "ABCDEFGHIJKLMNOPQRSTUVWXYZ" ),
  LOWERCASE( "abcdefghijklmnopqrstuvwxyz" ),
  NUMERIC( "1234567890" ),
  SPECIAL( "~!@#$%^&*()_|" );

  private String characters;

  public String toString()
    {
    return this.characters;
    }

  private AsciiCharacters( String characters )
    {
    this.characters = characters;
    }
  } // AsciiCharacters

Thus the users have the choice to:

  • use the predefined characters in any combination of them
  • supply their own characters

Example:

String useThisCharacters = 
  String.format( "%s%s", AsciiCharacters.UPPERCASE, AsciiCharacters.LOWERCASE );
new PasswordGenerator( useThisCharacters );
Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ I used your tips, from 1) to 6), thank you for your time. Do i correctly outsourced chars from PasswordGenerator? I mean, it's working, of course, but I doubt that UPPERCASE_CHARACTERS in PasswordGenerator.java can stay. Should I make PasswordGenerator.java more independent? I also have doubt about Dependency Injection - it is correct way to have default chars in PG.java? I cuted out file PasswordGenerator.java from project, added main function and launched it from command line - it is working correctly. \$\endgroup\$
    – nanomader
    Jun 8, 2015 at 14:27
  • 1
    \$\begingroup\$ @nanomader You're welcome. That's why we are here. :-) Looks good and great that you made it work. UPPERCASE_CHARACTERS in PG is fine since this is the default. I'd call this(UPPERCASE_CHARACTERS); (constructor chaining) in the default constructor. \$\endgroup\$
    – Gerold Broser
    Jun 8, 2015 at 14:46
  • 1
    \$\begingroup\$ Be careful with Math.pow() for it is very slow and inefficient for smaller powers. Here it does not matter, but in some loop difference between a*a*a*a and Math.pow(a,4) can be quite significant. Or at least it was this way last time I checked, not sure whether compiler approach changed in java 8. \$\endgroup\$
    – Vojtěch Kaiser
    Jun 9, 2015 at 19:34
4
\$\begingroup\$

Should i use try catch in, for example, button handling or password generating?

try-catch is for handling exceptions. For example in the middle of downloading a URL the network connection can get disrupted and trigger an exception, which can be handled gracefully by retrying to download. In your app, I don't see what can go wrong, I don't see what kind of exceptions might be triggered.

"Should I use try catch" is a very strange question to ask. Using try catch is typically not an option, but mandatory when it's needed. It's not something you ask yourself.

I saw some user answers who discouraged using try catch statement. I am reading a lot, and I see that everyone has own opinion.

I suspect there is a bigger context there when somebody discourages using try-catch. It doesn't make sense to say so, the statement has no meaning, without explaining the context where it would be true. Using try-catch is not a matter of opinion, the operations you're doing either force you to handle exceptions or don't, not your choice.

If i write another program, all by myself or in bigger part myself, can i post code here, even if we have here a lot of similar projects? Like my passwordGenerator - there are a lot of similar threads.

It's normal to have similar threads. Code you wrote yourself, to the best of your knowledge, fully working, and you would like it peer-reviewed, then this is a good place to post it.

Code review

The way you create the layout, with the deeply chained .addGroup, add-this, add-that, is a bit hard to read, but I don't know how to do that better. Maybe somebody else can advise you on that.

In the GUI class, many variables that are declared members should be local variables instead:

private javax.swing.JButton btnGenerate;
private javax.swing.ButtonGroup buttonGroup1;
private javax.swing.ButtonGroup buttonGroup2;
private javax.swing.ButtonGroup buttonGroup3;
private javax.swing.ButtonGroup buttonGroup4;
private javax.swing.JButton jButton2;
private javax.swing.JLabel jLabel1;
private javax.swing.JLabel jLabel2;
private javax.swing.JLabel jLabel4;
private javax.swing.JScrollPane jScrollPane1;
private static javax.swing.JSlider jSlider1;

It's good to limit the scope of variables to the minimum. The reason is simple: when a variable is not accessible in a bigger scope than necessary, it cannot be misused.

There is jLabel3 which is used by more than one method, so that cannot be converted to a local variable. However, if it's so different from the others, then it deserves a better name than "jLabel3", as it's clearly a bit special.

And why do you refer to classes with the complete package names instead of importing them? This is very tedious.

Judging by loadGUI, it looks like you're using Java 8. In which case you can simplify some anonymous classes, for example instead of:

    btnGenerate.addActionListener(new java.awt.event.ActionListener() {
        public void actionPerformed(java.awt.event.ActionEvent evt) {
            btnGenerateActionPerformed(evt);
        }
    });

You can write:

    btnGenerate.addActionListener(this::btnGenerateActionPerformed);

There are some other places where you could apply the sample logic, for example:

    jButton2.addActionListener(GUI.this::copyToClipboard);
Improve this answer
\$\endgroup\$
2
  • 4
    \$\begingroup\$ Just so you know, it looks like OP used net beans to generate his GUI, so that explains the fully qualified names and variables at the bottom. \$\endgroup\$
    – Kyranstar
    Jun 7, 2015 at 20:16
  • 2
    \$\begingroup\$ Thanks, that's good to know. I'll go easy on posts like that in the future ;-) \$\endgroup\$
    – janos
    Jun 7, 2015 at 20:17

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.