Swing calculator - first GUI program

Question

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));
        functionButtonPanel.add(enterButton);
        functionButtonPanel.add(cButton);
        functionButtonPanel.add(multiplyButton);
        functionButtonPanel.add(divideButton);
        functionButtonPanel.add(addButton);
        functionButtonPanel.add(substractButton);

    numberButtonsAction[] numberButtonActions = new numberButtonsAction[10];
    for ( int i = 0; i < 10; i++ ) {
        numberButtonActions[i] = new numberButtonsAction(numberButtons[i]);
        numberButtons[i].addActionListener(numberButtonActions[i]);
    }

    EnterButton enter = new EnterButton();
        enterButton.addActionListener(enter);

    CButton c = new CButton();
        cButton.addActionListener(c);

    MultiplyButton multiply = new MultiplyButton();
        multiplyButton.addActionListener(multiply);

    DivideButton divide = new DivideButton();
        divideButton.addActionListener(divide);

    AddButton add = new AddButton();
        addButton.addActionListener(add);

    SubtractButton subtract = new SubtractButton();
        substractButton.addActionListener(subtract);

    motherPanel.add(textPanel);
    motherPanel.add(numberButtonsPanel);
    motherPanel.add(functionButtonPanel);
    add(motherPanel);

    setTitle("ButtonTest");
    setSize(180, 290);
    setLocationByPlatform(true);
    setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
    setVisible(true);

}

private class numberButtonsAction implements ActionListener {

    private String c;

    public numberButtonsAction(JButton a) {
        this.c = a.getText();
    }

    public void actionPerformed(ActionEvent e) {
        if (!resultJText.getText().equals("0.0")) {
            resultJText.setText(resultJText.getText() + c);
        } else {
            resultJText.setText("");
            actionPerformed(e);
        }
    }
}

private class EnterButton implements ActionListener {

    @Override
    public void actionPerformed(ActionEvent e) {
        tempNumbers2 = Double.parseDouble(resultJText.getText());

        if (function == 0) {
            resultJText.setText(Double.toString((Math.round((tempNumbers1 / tempNumbers2) * 100)) / 100));
        } else if (function == 1) {
            resultJText.setText(Double.toString(tempNumbers1 * tempNumbers2));
        } else if (function == 2) {
            resultJText.setText(Double.toString(tempNumbers2 + tempNumbers1));
        } else if (function == 3) {
            resultJText.setText(Double.toString(tempNumbers1 - tempNumbers2));
        } else {
            resultJText.setText(String.valueOf(tempNumbers1));
        }
            tempNumbers1 = Double.parseDouble(resultJText.getText());
    }


}

private class CButton implements ActionListener {

    @Override
    public void actionPerformed(ActionEvent e) {
        resultJText.setText("");
        tempNumbers1 = 0;
        tempNumbers2 = 0;

        function = -1;
    }


}

private class DivideButton implements ActionListener {

    @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 = 0;
    }

}

private class MultiplyButton implements ActionListener {

    @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 = 1;
    }


}


private class AddButton implements ActionListener {

    @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 = 2;
    }

}

private class SubtractButton implements ActionListener {

    @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 = 3;
    }

}
}

Answer 1

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));
}

Answer 2

You are not modifying the behavior of JFrame so why would you extend it ? It is a "common" mistake with Swing. You should remove your extends JFrame for a private JFrame field in your class. This will change almost nothing to your code since all would do is instead of calling add directly, you will call it on your field. What will change is your will not be tie to JFrame so much. Waht if you want a JDialog instead of a JFrame ? Having a field will make this change easier. (In your case, you will need a JFrame but it could not always be the case).

There is something I like about your code, no absolute position! You don't know how much this is valuable to maintain a GUI! I deal with GUI with absolute position, and it's just horrible! Keep it that way. Use the Layout you need, don't be too restrictive in your design and NEVER (well not never) position your components in an absolute way.

---

Personally, I don't like when you mention the name of the class in a variable :

private JTextField resultJText;

If you ever change your class, oops you need to change your variable name or it will be inconsistent! You could go for resultText or simply result but you really don't need to mention that it's a JTextField.