Simple calculator in Java using Swing and AWT

Question

I wrote a simple calculator with general operations. Give me please some advice, suggestions, and criticism about my code: code design, readability, mistakes.

Calculator.java

import javax.swing.*;

public class Calculator {
    public static void main(String[] args) {
    CalculatorView calculator = new CalculatorView();

    // Windows settings
    calculator.setTitle("Simple Calculator");
    calculator.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
    }
}

CalculatorEngine.java

 public class CalculatorEngine {

    private enum Operator {
        ADD, SUBTRACT, MULTIPLY, DIVIDE
    }

    private double currentTotal;

    public String getTotalString() {
        return currentTotal % 1.0 == 0
                ? Integer.toString((int) currentTotal)
                : String.valueOf(currentTotal);
    }

    public void equal(String number) {
        currentTotal = Double.parseDouble(number);
    }

    public void add(String number) {
        convertToDouble(number, Operator.ADD);
    }

    public void subtract(String number) {
        convertToDouble(number, Operator.SUBTRACT);
    }

    public void multiply(String number) {
        convertToDouble(number, Operator.MULTIPLY);
    }

    public void divide(String number) {
        convertToDouble(number, Operator.DIVIDE);
    }

    private void convertToDouble(String number, Operator operator) {
        double dblNumber = Double.parseDouble(number);
        switch (operator) {
            case ADD:
                add(dblNumber);
                break;
            case SUBTRACT:
                subtract(dblNumber);
                break;
            case MULTIPLY:
                multiply(dblNumber);
                break;
            case DIVIDE:
                divide(dblNumber);
                break;
            default:
                throw new AssertionError(operator.name());
        }
    }

    private void add(double number) {
        currentTotal += number % 1.0 == 0 ? (int) number : number;
    }

    private void subtract(double number) {
        currentTotal -= number % 1.0 == 0 ? (int) number : number;
    }

    private void multiply(double number) {
        currentTotal *= number % 1.0 == 0 ? (int) number : number;
    }

    private void divide(double number) {
        currentTotal /= number % 1.0 == 0 ? (int) number : number;
    }

}

CalculatorView.java

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

public class CalculatorView extends JFrame {
    // Declaring fields
    private JTextField display;
    private static final Font BOLD_FONT = new Font(Font.MONOSPACED, Font.BOLD, 20);

    // Variables for calculator's state
    private boolean startNumber = true;                         // expecting number, not operation
    private String prevOperation = "=";                         // previous operation
    private CalculatorEngine engine = new CalculatorEngine();   // Reference to CalculatorEngine

    public CalculatorView() {

        // Window settings
        Dimension size = new Dimension(320, 300);
        setPreferredSize(size);
        setResizable(false);

        // Display field
        display = new JTextField("0", 18);
        display.setFont(BOLD_FONT);
        display.setHorizontalAlignment(JTextField.RIGHT);

        // Operations panel 1
        ActionListener operationListener = new OperationListener();
        JPanel operationPanel1 = new JPanel();
        String[] operationPanelNames1 = new String[]{"+", "-", "*", "/"};
        operationPanel1.setLayout(new GridLayout(2, 2, 2, 2));
        for (String anOperationPanelNames1 : operationPanelNames1) {
            JButton b = new JButton(anOperationPanelNames1);
            operationPanel1.add(b);
            b.addActionListener(operationListener);
        }

        // Operations panel 2
        JPanel operationPanel2 = new JPanel();
        operationPanel2.setLayout(new GridLayout(1, 1, 2, 2));
        JButton clearButton = new JButton("C");
        clearButton.addActionListener(new ClearKeyListener());
        operationPanel2.add(clearButton);
        JButton equalButton = new JButton("=");
        equalButton.addActionListener(operationListener);
        operationPanel2.add(equalButton);


        // Buttons panel
        JPanel buttonPanel = new JPanel();
        ActionListener numberListener = new NumberKeyListener();
        String[] buttonPanelNames = new String[]{"7", "8", "9", "4", "5", "6", "1", "2", "3", " ", "0", " "};
        buttonPanel.setLayout(new GridLayout(4, 3, 2, 2));
        for (String buttonPanelName : buttonPanelNames) {
            JButton b = new JButton(buttonPanelName);
            if (buttonPanelName.equals(" ")) {
                b.setEnabled(false);
            }
            b.addActionListener(numberListener);
            buttonPanel.add(b);
        }

        // Main panel
        JPanel mainPanel = new JPanel();
        mainPanel.setLayout(new BorderLayout());
        mainPanel.add(display, BorderLayout.NORTH);
        mainPanel.add(operationPanel1, BorderLayout.EAST);
        mainPanel.add(operationPanel2, BorderLayout.SOUTH);
        mainPanel.add(buttonPanel, BorderLayout.CENTER);

        // Window build
        setContentPane(mainPanel);
        pack();
        setVisible(true);
    }

    private void actionClear() {
        startNumber = true;
        display.setText("0");
        prevOperation = "=";
        engine.equal("0");
    }

    class OperationListener implements ActionListener {

        @Override
        public void actionPerformed(ActionEvent e) {
            if (startNumber) {
                actionClear();
                display.setText("ERROR - wrong operation");
            } else {
                startNumber = true;
                try {
                    String displayText = display.getText();
                    switch (prevOperation) {
                        case "=":
                            engine.equal(displayText);
                            break;
                        case "+":
                            engine.add(displayText);
                            break;
                        case "-":
                            engine.subtract(displayText);
                            break;
                        case "/":
                            engine.divide(displayText);
                            break;
                        case "*":
                            engine.multiply(displayText);
                            break;
                    }
                    display.setText("" + engine.getTotalString());
                } catch (NumberFormatException ex) {
                    actionClear();
                }
                prevOperation = e.getActionCommand();
            }
        }
    }

    class NumberKeyListener implements ActionListener {

        @Override
        public void actionPerformed(ActionEvent e) {
            String digit = e.getActionCommand();
            if (startNumber) {
                display.setText(digit);
                startNumber = false;
            } else {
                display.setText(display.getText() + digit);
            }
        }
    }

    class ClearKeyListener implements ActionListener {

        @Override
        public void actionPerformed(ActionEvent e) {
            actionClear();
        }
    }

}

Answer 1

Usability issues

Some things don't work as I would expect:

• Pressing the equals button twice in a row or after clearing gives "ERROR - wrong operation"
• Pressing an operation after the equal buttons (to continue calculations), gives "ERROR - wrong operation"

It would be good to optimize make the user interface a bit friendlier.

Separation of concerns

It's good that you separated the engine, the view, and the main class that just sets up and runs everything. But it would be good to go further.

The calculations are performed by the engine, and controlled by an action listener implemented inside the view, using a switch. Instead of a switch, it would be better to abstract the calculation logic, for example using an Operator interface with an apply method. The Calculator class could configure CalculatorView with an arbitrary collection of Operator implementations. In that setup, CalculatorView will not be aware of any of the calculation logic, it will just know that each operation implements Operator, and has an apply method to perform some calculation. That will be more flexible and extensible.

Naming

Many of the method and variable names are quite good, but there are some bad ones that stand out, for example in this code:

    for (String anOperationPanelNames1 : operationPanelNames1) {
        JButton b = new JButton(anOperationPanelNames1);
        operationPanel1.add(b);
        b.addActionListener(operationListener);
    }

anOperationPanelNames1 is the most terrible name in the code. b is not great either, spelling out to button would make it a tad more readable, and not terribly long.

There is operationPanel1 and operationPanel2, but they are quite different in nature. The first contains operators used in calculations, the second is more about controlling the application, which is different from performing calculations. So instead of numbering variables, you could give them more meaningful names.

Answer 2

The constructor for CalculatorView is quite lengthy, you should consider breaking it down to methods as indicated by your comments:

public CalculatorView() {
    // ...
    display = getDisplayField();
    JPanel operators = getOperatorsPanel();
    JPanel operations = getOperationsPanel();
    JPanel buttons = getButtonsPanel();
    // organize() creates your "main panel"
    setContentPane(organize(display, operators, operations, buttons));
    // "Windows settings" in Calculator can be done here too
    setTitle("Simple Calculator");
    setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE)
    pack();
    setVisible(true);
}

It is recommended to display your GUI window in the following way inside Calculator.main():

public static void main(String[] args) {
    // Java 8 lambda below
    SwingUtilities.invokeLater(CalculatorView::new);
}

The other thing I want to highlight is that your CalculatorEngine seems to be over-engineered. You use an enum to identify the different arithmetic expressions, which is nice and fine, but they are only used in a switch statement, with the actual calculations done in private add/subtract/multiply/divide methods, which are themselves called through public add/subtract/multiply/divide methods with the relevant enum value... That just sounds like too many steps to perform a calculation.

It might be easier if your Operator enum can aid in doing the calculations too, e.g. Operator.ADD knows how to add two double values together to return a double value. In Java 8, that would be an implementation of either BinaryOperator<Double> (using the Double wrapper) or DoubleBinaryOperator (using the double primitive). In that case, you wouldn't require the private add/subtract/multiply/divide methods in CalculatorEngine.

One small note for OperationListener.actionPerformed(ActionEvent):

display.setText("" + engine.getTotalString());

I suppose the "" + was added by mistake, and can be removed.