Java Swing Calculator

Question

I've been learning java for over a year and I'm looking for better ways to learn and I'm trying to get a better idea of where I am as a programmer. I honestly have no clue if my code is good or bad. I'd really appreciate it if you could have a look at my code and point out anything that's wrong with it. The calculator has the same functionality as the Windows 7 calculator and it's split into 3 classes.

import java.awt.BorderLayout;
import java.awt.Font;
import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.script.ScriptException;
import javax.swing.*;

public class Calculator extends JFrame implements Runnable {

private MemoryStore memoryStore = new MemoryStore();
private JTextPane screen;
private JPanel buttonPanel;
private JButton[] numberButtons;
private JButton[] operationButtons;
private String display = "";
private boolean shouldOverwrite = false;
private String[] operationButtonStrings = { "ME", "MR", "MS", "M+", "M-", "←", "C", "+", "-", "*", "/", "√", "x²",
        "±", "%", ".", "1/x", "=" };

public Calculator() {
    super("Java Calculator");
}

@Override
public void run() {
    makeGUI();
}

private void makeGUI() {
    frameSetup();
    screenSetup();
    createNumberButtons();
    createOperationButtons();
    addButtons();
    setVisible(true);
}

private void frameSetup() {
    setSize(300, 300);
    setLocationRelativeTo(null); // Open frame in middle of screen
    setResizable(false);
    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}

private void screenSetup() {
    screen = new JTextPane();
    screen.setBorder(BorderFactory.createBevelBorder(1));
    screen.setEditable(false);
    screen.setFont(new Font("Segoe UI", Font.BOLD, 18));
    add(screen, BorderLayout.NORTH);
}

private void createNumberButtons() {
    MyNumberButtonListener numberButtonListener = new MyNumberButtonListener();
    numberButtons = new JButton[10];
    for (int i = 0; i < 10; i++) {
        numberButtons[i] = new JButton(Integer.toString(i));
        numberButtons[i].addActionListener(numberButtonListener);
        numberButtons[i].setFocusable(false);
    }
}

private void createOperationButtons() {
    MyOperationButtonListener operationButtonListener = new MyOperationButtonListener();
    operationButtons = new JButton[18];
    for (int i = 0; i < 18; i++) {
        operationButtons[i] = new JButton(operationButtonStrings[i]);
        operationButtons[i].addActionListener(operationButtonListener);
        operationButtons[i].setFocusable(false);
    }

}

private void addButtons() {
    buttonPanel = new JPanel();
    buttonPanel.setLayout(new GridLayout(6, 5, 1, 1));
    buttonPanel.setBorder(BorderFactory.createEmptyBorder(5, 5, 5, 5));

    for (int i = 0; i < 10; i++) {
        buttonPanel.add(numberButtons[i]);
    }
    for (int i = 0; i < 18; i++) {
        buttonPanel.add(operationButtons[i]);
    }

    buttonPanel.add(new JButton());
    buttonPanel.add(new JButton());
    add(buttonPanel);

}

private class MyNumberButtonListener implements ActionListener {
    @Override
    public void actionPerformed(ActionEvent e) {

        for (int i = 0; i < 10; i++) {
            if (e.getSource() == numberButtons[i]) {
                displayNumber(i);
                break;
            }
        }

    }

}

private void displayNumber(int number) {
    if (shouldOverwrite) {
        screen.setText("" + number);
        shouldOverwrite = false;
    } else {
        display = screen.getText();
        screen.setText(display + number);
    }
}

private class MyOperationButtonListener implements ActionListener {
    @Override
    public void actionPerformed(ActionEvent e) {

        if (e.getSource() == operationButtons[0]) {
            memoryStore.resetStoredValue();
        } else if (e.getSource() == operationButtons[1]) {
            display = screen.getText();
            screen.setText(display + "" + memoryStore.getStoredValue());
        } else if (e.getSource() == operationButtons[2]) {
            if (!screenIsEmpty())
                memoryStore.store(readScreenAsDouble());
        } else if (e.getSource() == operationButtons[3]) {
            if (!screenIsEmpty())
                memoryStore.plusStoredValue(readScreenAsDouble());
        } else if (e.getSource() == operationButtons[4]) {
            if (!screenIsEmpty())
                memoryStore.minusStoredValue(readScreenAsDouble());
        } else if (e.getSource() == operationButtons[5]) {
            backSpace();
        } else if (e.getSource() == operationButtons[6]) {
            clearScreen();
        } else if (e.getSource() == operationButtons[7]) {
            displaySymbol(operationButtons[7].getText());
        } else if (e.getSource() == operationButtons[8]) {
            displaySymbol(operationButtons[8].getText());
        } else if (e.getSource() == operationButtons[9]) {
            displaySymbol(operationButtons[9].getText());
        } else if (e.getSource() == operationButtons[10]) {
            displaySymbol(operationButtons[10].getText());
        } else if (e.getSource() == operationButtons[11]) {
            writeToScreen(CalcUtilities.sqrRoot(readScreenAsDouble()));
            shouldOverwrite = true;
        } else if (e.getSource() == operationButtons[12]) {
            writeToScreen(CalcUtilities.squared(readScreenAsDouble()));
            shouldOverwrite = true;
        } else if (e.getSource() == operationButtons[13]) {
            changeToPlusOrMinus();
        } else if (e.getSource() == operationButtons[14]) {
            displaySymbol(operationButtons[14].getText());
        } else if (e.getSource() == operationButtons[15]) {
            if (!displayHasDecimal())
                displaySymbol(operationButtons[15].getText());
        } else if (e.getSource() == operationButtons[16]) {
            writeToScreen(CalcUtilities.reciprocal(readScreenAsDouble()));
            shouldOverwrite = true;
        } else if (e.getSource() == operationButtons[17]) {
            equalsAction();
        }

    }

}

private double readScreenAsDouble() {
    double result;
    display = screen.getText();
    result = parseDisplayToDouble(display);
    return result;
}

private void writeToScreen(double numToWrite) {
    screen.setText(Double.toString(numToWrite));
}

public double parseDisplayToDouble(String display) {
    double result;
    try {
        result = Double.parseDouble(display);
    } catch (NumberFormatException e) {
        return 0.0;
    }
    return result;
}

private void displaySymbol(String symbol) {
    display = screen.getText();
    screen.setText(display + symbol);
    shouldOverwrite = false;
}

private void clearScreen() {
    screen.setText("");
}

private boolean displayHasDecimal() {
    display = screen.getText();
    if (display.indexOf('.') != -1)
        return true;
    else
        return false;
}

private void equalsAction() {
    display = screen.getText();
    try {
        screen.setText(CalcUtilities.evaluate(display) + "");
    } catch (ScriptException e) {
        screen.setText("Invalid");
    }
    shouldOverwrite = true;
}

private void backSpace() {
    if (!screenIsEmpty()) {
        display = screen.getText();
        screen.setText(display.substring(0, display.length() - 1));
    }

}

private void changeToPlusOrMinus() {
    display = screen.getText().toString();
    if (display.charAt(0) != '-')
        screen.setText("-" + display);
    else
        screen.setText(display.substring(1, display.length()));
    shouldOverwrite = false;
}

private boolean screenIsEmpty() {
    display = screen.getText().toString();
    if (display.equals(""))
        return true;
    else
        return false;
}

}

/*
 * Author: Alex Matthews
 * Ireland
 */ 
import javax.script.*;

public class CalcUtilities {

private static ScriptEngineManager mgr = new ScriptEngineManager();
private static ScriptEngine engine = mgr.getEngineByName("JavaScript");

public static double reciprocal(double num) {
    return 1 / num; 
}


public static double squared(double num) {
    return num * num;
}

public static double sqrRoot(double num) {
    return Math.sqrt(num);
}

public static Object evaluate(String equation) throws ScriptException {
    return engine.eval(equation);
}
}

/*
 * Author: Alex Matthews
 * Ireland
 */ 
public class MemoryStore {

    private double storedVal = 0.0;

    public void store(double num) {
        storedVal = num;
    }

    public double getStoredValue() {
        return storedVal;
    }

    public void resetStoredValue() {
        storedVal = 0.0;
    }

    public void plusStoredValue(double num) {
        storedVal += num;
    }

    public void minusStoredValue(double num) {
        storedVal -= num;
    }

}

Answer 1

Pros:

• basic separation of concerns:

  • you have separate Listeners for Number buttons and operator buttons
  • you created small methods with limited concerns.
  • you created two arrays to hold buttons of same type but different business roles.

• Naming: you follow the Java naming conventions and your identifier have useful names.

• comments: you have a comment which tells why the code is at that line. You have no other comments.

cons:

inheritance misconception:

Your class Calculator extends JFrame, but it does not add new functionality to it, it just configures the frame.

if/else cascade instead of polymorphism

The method actionPerformed() Your class MyOperationButtonListener contains a long if/else cascade. This is because you have to know which button caused the event.

This could be avoided if you had a separate Listener instance for each button. This does not mean that a separate Listener class is needed for each button. The listeners should have been created as anonymous inner classes of the ActionListener interface directly.

The downside of it that you cannot create the operator buttons in a loop anymore, but this is ok because each button ha a different behavior:

private void createOperationButtons() {
    operationButtons = new JButton[18];
        operationButtons[0] = new JButton("ME"); // the array operationButtonStrings is not needed.
        operationButtons[0].addActionListener(new ActionListener(){
           public void actionPerformed(ActionEvent e) {
              memoryStore.resetStoredValue();
           }
        });    
        operationButtons[0].setFocusable(false);

        operationButtons[1] = new JButton("MR"); // the array operationButtonStrings is not needed.
        operationButtons[1].addActionListener((e) -> { // java8 lambda version
             display = screen.getText();
             screen.setText(display + "" + memoryStore.getStoredValue());
        });    
        operationButtons[1].setFocusable(false);
       // ...
    }
}

To reduce the code duplication youcould extract the actual buton cration and configuration in an extra method:

private JButton createButton(String text, ActionListener action){
    JButton button = new JButton(text);
    button.addActionListener(action);
    button.setFocusable(false);
    return button;
}

private void createOperationButtons() {
    operationButtons = new JButton[18];
        operationButtons[0] = createButton("ME",(e)->memoryStore.resetStoredValue());
        operationButtons[1] = createButton("MR",(e)->{
             display = screen.getText();
             screen.setText(display + "" + memoryStore.getStoredValue());
        });
    // ....
}

As you can see this brings the "name" and the "function" closer together and does not need any if/else cascade or switch...

---

I have to do it 18 times in one method it makes the method very big and ugly. There has to be a way of making it nice and clean. – Alex. M

You cannot avoid having to specify the different behavior for the operator buttons.

In your first solution the ugliness was the if/else cascade in your actionPerformed() method.

With my suggestion you could at least ease the readers pain by having an additional method for each operator button:

private JButton createButton(String text, ActionListener action){
    JButton button = new JButton(text);
    button.addActionListener(action);
    button.setFocusable(false);
    return button;
}

private JButton createMemoryEraseButton(){
    return createButton("ME",(e)->memoryStore.resetStoredValue());
}

private JButton createMemoryRecallButton(){
    return createButton("MR",(e)->{
             display = screen.getText();
             screen.setText(display + "" + memoryStore.getStoredValue());
        });
}
// ....

private JButton[] createOperationButtons() {
    operationButtons = new JButton[] {
       createMemoryEraseButton(),
       createMemoryRecallButton(),
       // ....
    };
    return operationButtons;
}

You could even go one step further and move the method createOperationButtons() and all the create??Button() methods in to a separate class.

Off cause, all the objects these methods work on you would have to pass in as prarameters either to the constructor of the new class or the createOperationButtons() method and subsequently to the create??Button() methods as needed.