patternjavaModerate
Swing calculator - first GUI program
Viewed 0 times
programswingfirstguicalculator
Problem
I've recently made a calculator using Java and Swing. I'm okay with the results but I'm curious what are the major flaws I've made (assuming there are, because it is my first program making GUI). I can't think of a fancy way for not duplicating code in the function action listeners classes.
Which other good-practice-rules have I broken?
Pastebin link
```
import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
public class Frame extends JFrame {
private double tempNumbers1 = 0;
private double tempNumbers2 = 0;
private byte function = -1;
private JTextField resultJText;
public Frame() {
JButton[] numberButtons = new JButton[10];
for ( int i = 9; i >= 0; i--) {
numberButtons[i] = new JButton(Integer.toString(i));
}
JButton enterButton = new JButton("Enter");
JButton cButton = new JButton("C");
JButton multiplyButton = new JButton("*");
JButton divideButton = new JButton("/");
JButton addButton = new JButton("+");
JButton substractButton = new JButton("-");
resultJText = new JTextField();
resultJText.setPreferredSize(new Dimension(160, 20));
resultJText.setBackground(Color.WHITE);
resultJText.setEnabled(false);
resultJText.setHorizontalAlignment(4);
resultJText.setDisabledTextColor(Color.BLACK);
JPanel motherPanel = new JPanel();
motherPanel.setLayout(new BoxLayout(motherPanel, BoxLayout.Y_AXIS));
JPanel textPanel = new JPanel();
textPanel.setPreferredSize(new Dimension(160, 20));
textPanel.add(resultJText);
JPanel numberButtonsPanel = new JPanel();
numberButtonsPanel.setPreferredSize(new Dimension(160, 100));
for(int i = 9; i>=0; i--) {
numberButtonsPanel.add(numberButtons[i]);
}
JPanel functionButtonPanel = new JPanel();
functionButtonPanel.setPreferredSize(new Dimension(160, 35));
functionButton
Which other good-practice-rules have I broken?
Pastebin link
```
import javax.swing.*;
import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
public class Frame extends JFrame {
private double tempNumbers1 = 0;
private double tempNumbers2 = 0;
private byte function = -1;
private JTextField resultJText;
public Frame() {
JButton[] numberButtons = new JButton[10];
for ( int i = 9; i >= 0; i--) {
numberButtons[i] = new JButton(Integer.toString(i));
}
JButton enterButton = new JButton("Enter");
JButton cButton = new JButton("C");
JButton multiplyButton = new JButton("*");
JButton divideButton = new JButton("/");
JButton addButton = new JButton("+");
JButton substractButton = new JButton("-");
resultJText = new JTextField();
resultJText.setPreferredSize(new Dimension(160, 20));
resultJText.setBackground(Color.WHITE);
resultJText.setEnabled(false);
resultJText.setHorizontalAlignment(4);
resultJText.setDisabledTextColor(Color.BLACK);
JPanel motherPanel = new JPanel();
motherPanel.setLayout(new BoxLayout(motherPanel, BoxLayout.Y_AXIS));
JPanel textPanel = new JPanel();
textPanel.setPreferredSize(new Dimension(160, 20));
textPanel.add(resultJText);
JPanel numberButtonsPanel = new JPanel();
numberButtonsPanel.setPreferredSize(new Dimension(160, 100));
for(int i = 9; i>=0; i--) {
numberButtonsPanel.add(numberButtons[i]);
}
JPanel functionButtonPanel = new JPanel();
functionButtonPanel.setPreferredSize(new Dimension(160, 35));
functionButton
Solution
tempNumbers isn't a very explanatory variable name. Let the variable itself describe what it is temp for. For a simple calculator though, perhaps number1 and number2 would suffice.You're using strange indentation in your constructor. It's good that you separate the fields, but no need to add an extra indentation for the lines where you call methods on the buttons.
The class
numberButtonsAction should be named with capital N to be consistent with the Java coding conventions.You have this
if in your code:if (!resultJText.getText().equals("0.0")) {But when you clear that textfield, you don't set it to
"0.0", you set it to an empty string:resultJText.setText("");I am not sure how it all works out when running it, but it is a bit suspicious to me. Perhaps you would want to parse the text as a double (set it to
0.0 if it's not a number), check the value, and compare using the value instead of the text. Just an idea though.For the different operations, you should consider using the Strategy Pattern.
You can use an enum instead of a byte.
public enum CalcFunction {
ADDITION, SUBTRACTION, MULTIPLICATION, DIVISION;
}If you would use Java 8, you could use the
DoubleBinaryOperator interface and lambdas. You could also make the whole ActionListener things a lot smoother by using Java method references. button.addActionListener(this::divideButtonClick); (I love Java 8!)The current class names
DivideButton, MultiplyButton etc sounds like they extend JButton, but they don't (and they shouldn't so that's good). Better names would be adding Listener at the end, however, if you are unable to use Java 8, you can create a common class like this:private class CalculationListener implements ActionListener {
private final CalcFunction operation;
public CalculationListener(CalcFunction function) {
this.operation = function;
}
@Override
public void actionPerformed(ActionEvent e) {
if (tempNumbers1 == 0) {
tempNumbers1 = Double.parseDouble(resultJText.getText());
resultJText.setText("");
} else {
tempNumbers2 = Double.parseDouble(resultJText.getText());
resultJText.setText("");
}
function = operation;
}
}And use it like this:
divideButton.addActionListener(new CalculationListener(CalcFunction.DIVISION));Currently, you're using one loop to create the buttons:
for ( int i = 9; i >= 0; i--) {
numberButtons[i] = new JButton(Integer.toString(i));
}And then you have one loop to add the buttons to the panel:
for(int i = 9; i>=0; i--) {
numberButtonsPanel.add(numberButtons[i]);
}And another, a bit further down, to create the listeners:
numberButtonsAction[] numberButtonActions = new numberButtonsAction[10];
for ( int i = 0; i < 10; i++ ) {
numberButtonActions[i] = new numberButtonsAction(numberButtons[i]);
numberButtons[i].addActionListener(numberButtonActions[i]);
}You don't need all these three loops, simply one is enough. This will also make the arrays you use unnecessary. There's no need to store them in an array at all.
for ( int i = 9; i >= 0; i--) {
JButton numberButton = new JButton(Integer.toString(i));
numberButtonsPanel.add(numberButton);
numberButton.addActionListener(new NumberButtonsAction(numberButton));
}Code Snippets
if (!resultJText.getText().equals("0.0")) {resultJText.setText("");public enum CalcFunction {
ADDITION, SUBTRACTION, MULTIPLICATION, DIVISION;
}private class CalculationListener implements ActionListener {
private final CalcFunction operation;
public CalculationListener(CalcFunction function) {
this.operation = function;
}
@Override
public void actionPerformed(ActionEvent e) {
if (tempNumbers1 == 0) {
tempNumbers1 = Double.parseDouble(resultJText.getText());
resultJText.setText("");
} else {
tempNumbers2 = Double.parseDouble(resultJText.getText());
resultJText.setText("");
}
function = operation;
}
}divideButton.addActionListener(new CalculationListener(CalcFunction.DIVISION));Context
StackExchange Code Review Q#54720, answer score: 10
Revisions (0)
No revisions yet.