Skip to main content
Code Review

Return to Answer

added 7 characters in body
Source Link
Simon Forsberg
Simon Forsberg
  • 59.8k
  • 9
  • 158
  • 312
private class CalculationListener implements ActionListener {

    private final CalcFunction operation;

    public SomeFunctionCalculationListener(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;
    }
}

private class CalculationListener implements ActionListener {

    private final CalcFunction operation;

    public SomeFunction(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;
    }
}

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;
    }
}
added 1072 characters in body
Source Link
Simon Forsberg
Simon Forsberg
  • 59.8k
  • 9
  • 158
  • 312

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

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));
}
Source Link
Simon Forsberg
Simon Forsberg
  • 59.8k
  • 9
  • 158
  • 312

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 SomeFunction(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));