Eliminating global variables in GUI program

Question

I'm just getting started on GUI programming using swing in Java, and I would love some critiques on my code. I understand there is a danger in using global variables. If someone can hint me on how to get rid of my globals (which will be later registered for event handling), I'd really appreciate it.

import java.awt.BorderLayout;
import java.awt.FlowLayout;
import java.awt.GridLayout;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.SwingConstants;
public final class Calculator extends JFrame
{
 //initialise various variables for use within the program
 //BUTTONS
 private final JButton additionButton = new JButton("+"); 
 private final JButton subtractionButton = new JButton("-");
 private final JButton divisionButton = new JButton("/");
 private final JButton multiplicationButton = new JButton("*"); 
 //PANELS
 private JPanel operatorPanel;
 private JPanel operandPanel;
 //LABELS
 private JLabel operationLabel; 
 //constructor to initialise the frame and add components into it
 public Calculator()
 {
 super("Clancy's Calculator");
 setLayout(new BorderLayout(5, 10));
 setResizable(false);
 setSize(370, 200);
 setDefaultCloseOperation(EXIT_ON_CLOSE);
 setLocationRelativeTo(null);
 setVisible(true);
 //create a message label to display the operation that has just taken place
 operationLabel = new JLabel("YOU HAVE PERFORMED SOME OPERATION", SwingConstants.CENTER);
 add(getOperatorPanel(), BorderLayout.NORTH);
 add(getOperandPanel(), BorderLayout.CENTER);
 add(operationLabel, BorderLayout.SOUTH);
 pack();
 }
 //setter method for the operator panel
 public void setOperatorPanel()
 {
 operatorPanel = new JPanel();
 operatorPanel.setLayout(new FlowLayout());
 operatorPanel.add(additionButton);
 operatorPanel.add(subtractionButton);
 operatorPanel.add(multiplicationButton);
 operatorPanel.add(divisionButton);
 }
 //getter method for the operator panel
 public JPanel getOperatorPanel()
 {
 setOperatorPanel();
 return operatorPanel;
 }
 //setter method for operands panel
 public void setOperandPanel()
 {
 operandPanel = new JPanel();
 operandPanel.setLayout(new GridLayout(3, 2, 5, 5));
 //LABELS
 JLabel operandOneLabel = new JLabel("Enter the first Operand: ");
 JLabel operandTwoLabel = new JLabel("Enter the second Operand: ");
 JLabel answerLabel = new JLabel("ANSWER: ");
 //TEXT FIELDS
 JTextField operandOneText = new JTextField(); //retrieves one operand
 JTextField operandTwoText = new JTextField(); //retrieves another operand
 JTextField answerText = new JTextField(); //displays answer
 answerText.setEditable(false); //ensure the answer field is not editable
 operandPanel.add(operandOneLabel);
 operandPanel.add(operandOneText);
 operandPanel.add(operandTwoLabel);
 operandPanel.add(operandTwoText);
 operandPanel.add(answerLabel);
 operandPanel.add(answerText);
 }
 //getter method for operand panel
 public JPanel getOperandPanel()
 {
 setOperandPanel();
 return operandPanel;
 }
 /** main method */
 public static void main(String[] args)
 {
 new Calculator();
 }
}

Answer 1

A thing I like to do is instead of using four different JButtons is use an array of four JButtons that is initialized in the same method that you build the JPanel this would remove the need to have them as global variables. Also instead of using two different methods to create your JPanels just use one that returns a JPanel as you can see you can remove the JButtons and the two JPanels this way:

public JPanel buildOperatorPanel()
{
 JPanel operatorPanel = new JPanel();
 operatorPanel.setLayout(new FlowLayout());
 JButton[] buttons = new JButton[4];//the array of JButtons
 String[] buttonNames = {"+", "-", "/", "*"};//the names to be displayed on the JButtons
 for(int i = 0; i < buttons.length; i++)
 {
 buttons[i] = new JButton(buttonNames[i]);
 //buttons[i].addActionListener(yourActionListener)//for whenever you add event handling
 operatorPanel.add(buttons[i]);
 }
 return operatorPanel;
}

Another thing I could suggest is instead of named reference variables for your JLabels use anonymous objects so for operationLabel (this is only assuming your not going to use the JLabels later):

add(new JLabel("YOU HAVE PERFORMED SOME OPERATION", SwingConstants.CENTER), BorderLayout.SOUTH);

However I am going to suggest that you move the JTextFields to global variables. Based on what I'm assuming you have to do with this calculator you are going to want the JTextFields to be accessible to everything. Here is my revision of your code:

import java.awt.BorderLayout;
import java.awt.FlowLayout;
import java.awt.GridLayout;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.SwingConstants;
public final class Calculator extends JFrame
{
 private JTextField operandOneText;//these get initialized in buildOperandPanel() method which is called in the constructor
 private JTextField operandTwoText;
 private JTextField answerText;
 //constructor to initialise the frame and add components into it
 public Calculator()
 {
 super("Clancy's Calculator");
 setLayout(new BorderLayout(5, 10));
 setResizable(false);
 setSize(370, 200);
 setDefaultCloseOperation(EXIT_ON_CLOSE);
 setLocationRelativeTo(null);
 setVisible(true);
 add(buildOperatorPanel(), BorderLayout.NORTH);
 add(buildOperandPanel(), BorderLayout.CENTER);
 add(new JLabel("YOU HAVE PERFORMED SOME OPERATION", SwingConstants.CENTER), BorderLayout.SOUTH);
 pack();
 }
 //method builds and returns operatorPanel
 public JPanel buildOperatorPanel()
 {
 JPanel operatorPanel = new JPanel();//JPanels are FlowLayout by default
 JButton[] buttons = new JButton[4];
 String[] buttonNames = {"+", "-", "/", "*"};
 for(int i = 0; i < buttons.length; i++)
 {
 buttons[i] = new JButton(buttonNames[i]);//the array of JButtons
 //buttons[i].addActionListener(yourActionListener)//for whenever you add event handling
 operatorPanel.add(buttons[i]);
 }
 return operatorPanel;
 }
 //method builds and returns operandPanel
 public JPanel buildOperandPanel()
 {
 JPanel operandPanel = new JPanel();
 operandPanel.setLayout(new GridLayout(3, 2, 5, 5));
 //Initialize TEXT FIELDS
 operandOneText = new JTextField(); //retrieves one operand
 operandTwoText = new JTextField(); //retrieves another operand
 answerText = new JTextField(); //displays answer
 answerText.setEditable(false); //ensure the answer field is not editable
 operandPanel.add(new JLabel("Enter the first Operand: "));
 operandPanel.add(operandOneText);
 operandPanel.add(new JLabel("Enter the second Operand: "));
 operandPanel.add(operandTwoText);
 operandPanel.add(new JLabel("ANSWER: "));
 operandPanel.add(answerText);
 return operandPanel;
 }
 /** main method */
 public static void main(String[] args)
 {
 new Calculator();
 }
}

Answer 2

One of the reasons for removing global variables is to limit the tight coupling between pieces of an application. For example, if you reduce the coupling between the computation of the Calculations and the input and display of the operands and results, it would be easier to adapt your code for calculations to Web based application without rewriting and introducing new bugs in the calculations code.

In your case I would like to see a clearly defined 'interface', based only upon very primitive objects like strings or integers, between the Users inputs and the Calculation engine code.

So instead of referring to a global JTextField.Text to get an operand, the Calculations engine has a function that accepts a String and only a String. Then you have a small piece that says CalculatePlus(oJTextFieldOperandOne.Text).

Answer 3

First a bit of terminology. According to the Wikipedia definition, your program does not contain any global variables. The JButtons and JPanels that I think you are asking about are member variables of the Calculator class. That said, it is reasonable to ask if you can get rid of those variables. There is a rule of thumb that variables should be scoped as narrowly as possible, and this is one way of applying that.

Member variables are part of an object's state, and are accessible from any (non-static) member function. If that is not a requirement for a variable, then it doesn't have to be (and probably shouldn't be) a member variable. Currently, your program just builds the UI, so the existing functionality doesn't require that any of your buttons, panels, etc be accessible from outside the function that creates them and adds them to the GUI. Even when you hook up event handlers for the buttons, that can happen at the same time that they are created and so they don't need to be members.

As you add functionality, you'll find that you need to access certain widgets and it would be convenient to have them as a member. For instance, having the answerText text field available as a member would make it easy to write answers to the screen. And in applications like this there are generally conditions under which some buttons should become disabled; this too is much easier if those buttons are stored as member variables.

In general, the panels and other more structural UI elements (as opposed to functional ones like buttons and text boxes) rarely have reason to be stored as members. But every rule has exceptions.

Answer 4

Good feedback by user27517. I want to add one thing, which is to use SwingUtilities.invokeLater() in the main method:

public static void main(String[] args) 
{
 SwingUtilities.invokeLater(new Runnable() 
 {
 public void run() 
 {
 new Calculator();
 }
 });
}

Why it's needed