Java Swing Calculator

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++) {
numberBut

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.

StackExchange Code Review Q#152655, answer score: 4