Number-guessing game in Java with Swing UI

I am trying not to pick up any bad habits while improving the way I write programs. Any suggestions on the following lines of code?

```
package guessGame;

import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.Random;

// Main Class
public class GuessGame extends JFrame {

// Declare class variables
private static final long serialVersionUID = 1L;
public static Object prompt1;
private JTextField userInput;
private JLabel comment = new JLabel(" ");
private JLabel comment2 = new JLabel(" ");
private int randomNumber;
private int counter = 0;

// Constructor
public GuessGame() {
super("Guessing Game");
setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);

// Content pane
setLayout(new FlowLayout());
Container c = getContentPane();

// Create components
JButton guessButton = new JButton("Try the number");
JButton newGameButton = new JButton("New Game");
JButton quitButton = new JButton("Quit");
JLabel prompt1 = new JLabel("I have a number between 1 and 1000.");
JLabel prompt2 = new JLabel("Can you guess the number?");
JLabel prompt3 = new JLabel("Please enter your guess: ");
comment = new JLabel(" ");
comment2 = new JLabel(" ");
userInput = new JTextField(5);

// Adding components to the pane
c.add(prompt1);
c.add(prompt2);
c.add(prompt3);
c.add(userInput);
c.add(guessButton);
c.add(newGameButton);
c.add(quitButton);
c.add(comment);
c.add(comment2);

// Set the mnemonic
guessButton.setMnemonic('T');
newGameButton.setMnemonic('N');
quitButton.setMnemonic('Q');

// Format pane
setSize(300, 200);
setLocationRelativeTo(null);
setVisible(true);
setResizable(false);

initializeNumber();

// Create the button handlers
GuessButtonHandler ghandler = new GuessButtonHandler(); // instantiate
// new object

Problems I see:

-
comment and comment2 are initialised twice, once in the class definition and again in the constructor.

-
If you click the New Game button, you'll get a new window but the old one will still be there. (see also the last point below)

You may want to provide a reset() method that will both initialise a random number and also reset the comment labels to their default values. Then you can call reset() both from your constructor and also when a game is done. Or even when a new game is asked for.

-
prompt1 is declared as a class member, but never used. You can get rid of it at the class level.

Try to find repeated parts of code, and think about how you can pull those out to a reusable method. Keep your code DRY.

Some pro-level tips and style-oriented things:

-
I usually like to make Swing components as self-contained as possible (see examples below). Therefore, for any UI-specific logic (such as internal event listening) I have the component (your JFrame) implement the listener interface (e.g., ActionListener for button clicks). If that triggers some type of application event, I'd fire off that type of event from within the actionPerformed() method of the UI component. The only time I extract an explicit separate class for Swing listeners is when they're generally useful across several UI components, which isn't real often.

This helps to isolate the UI as being Swing-based; the rest of your application receives an application-level event and it doesn't need to know if it was from a Swing button, a web button, a service invocation, or whatever. (You shouldn't be importing Swing interfaces into non-UI classes).

For trivial Swing applications like this, using inner classes to implement listeners is fine, and indeed it's a common practice. It just seems more clean (to me at least) to make a self-contained component out of it. For large Swing applications, however, all those extra classes can eat up your PermGen memory.

-
For convenience, the JFrame.add() method targets the content pane, so it's not strictly necessary to reference it directly. But it's good to remember that you need to deal with the content pane.

-
I also typically localise (small-L) the configuration of my internal components instead of spreading them out:

JButton button = new JButton("Try the number");
button.setMnemonic('T');
button.addActionListener(this);
add(button);

Now I can reuse the button variable name for the next one. Same with labels. After you create a couple of panels or frames, you'll start to notice some common coding patterns you use. Those can be extracted into static methods in a utility class. For instance, you could have a static method that creates and returns a new button given some text, a mnemonic, a command, and a listener.

Doing some of that, it might look like:

public static JButton mkButton(String text, Character mnemonic, String command, ActionListener listener) {
  final JButton button = new JButton(text);
  if (mnemonic != null) button.setMnemonic(mnemonic);
  if (command != null) button.setActionCommand(command);
  if (listener != null) button.addActionListener(listener);
  return button;
}

These utility methods really start to shine when you start using more advanced layout managers. Next, create constants for your commands in your class:

private static final String ACTION_GUESS = "guess";
private static final String ACTION_NEW = "new";
private static final String ACTION_QUIT = "quit";

then, in the constructor it's just:

add(mkButton("Try the number", 'T', ACTION_GUESS, this));

finally, you can switch on the action command of your button to either do trivial work in the switch or dispatch to a method for more complex operations:

@Override
public void actionPerformed(ActionEvent e) {
  switch(e.getActionCommand()) {
    case ACTION_GUESS:
      guess();
      break;

    case ACTION_QUIT:
      System.exit(0);
      break;

    case ACTION_NEW:
      reset();
      break;
  }
}

-
Having said all of the above concerning buttons, if you're building a large complex Swing app, you'll want to define javax.swing.Actions and reuse them in buttons and menus.

-
After you add all of your components, call pack(). That will set up dimensions if you want to tweak them (as opposed to setting absolute dimensions as you have).

-
You definitely don't want to setVisible(true) (or other side-effects) in a constructor. Let calling code determine when to display UI components.

-
Unless you know absolutely that your frame will only ever be used from main(), don't set the default close operation in the constructor; let the calling code set that (in this case, main()).

StackExchange Code Review Q#32104, answer score: 3