2
\$\begingroup\$

I'm learning Java and I've tried to write my own calculator with GUi based on this post. Here is the repo: https://github.com/MurDaD/java-calculator

Could someone check my code and tell me how to improve it?

The main problem I thing is in the Mediator class - it's huge, but I don't know what is the best way to refactor it.

Mediator.java

package com.murdad.GUI;

import com.murdad.GUI.numbers.*;
import com.murdad.GUI.operations.*;
import com.murdad.calculator.Calculator;
import com.murdad.utils.Utils;

import javax.swing.*;
import java.awt.*;

public class Mediator implements MediatorInterface {

    // TODO: change numbers String format to BigDecimal
    private String number1;
    private String number2;
    private int operation = 0;
    // TODO: Implement memory
    private String memory;
    private boolean clearOnNextNumber = false;

    private JTextField textField;
    private JButton buttonOne;
    private JButton buttonTwo;
    private JButton buttonThree;
    private JButton buttonFour;
    private JButton buttonFive;
    private JButton buttonSix;
    private JButton buttonSeven;
    private JButton buttonEight;
    private JButton buttonNine;
    private JButton buttonZero;
    private JButton buttonDel;
    private JButton buttonDot;
    private JButton buttonPlus;
    private JButton buttonMinus;
    private JButton buttonMultiply;
    private JButton buttonDivide;
    private JButton buttonEquals;
    private JButton buttonAllClear;

    @Override
    public void registerComponent(Component component) {
        System.out.println("Registering component " + component.getName());
        component.setMediator(this);
        switch (component.getName()) {
            case "ButtonOne":
                buttonOne = (ButtonOne)component;
                break;
            case "ButtonTwo":
                buttonTwo = (ButtonTwo)component;
                break;
            case "ButtonThree":
                buttonThree = (ButtonThree)component;
                break;
            case "ButtonFour":
                buttonFour = (ButtonFour)component;
                break;
            case "ButtonFive":
                buttonFive = (ButtonFive)component;
                break;
            case "ButtonSix":
                buttonSix = (ButtonSix)component;
                break;
            case "ButtonSeven":
                buttonSeven = (ButtonSeven)component;
                break;
            case "ButtonEight":
                buttonEight = (ButtonEight)component;
                break;
            case "ButtonNine":
                buttonNine = (ButtonNine)component;
                break;
            case "ButtonZero":
                buttonZero = (ButtonZero)component;
                break;
            case "ButtonDot":
                buttonDot = (ButtonDot)component;
                break;
            case "ButtonDel":
                buttonDel = (ButtonDel)component;
                break;
            case "ButtonPlus":
                buttonPlus = (ButtonPlus)component;
                break;
            case "ButtonMinus":
                buttonMinus = (ButtonMinus)component;
                break;
            case "ButtonMultiply":
                buttonMultiply = (ButtonMultiply)component;
                break;
            case "ButtonDivide":
                buttonDivide = (ButtonDivide)component;
                break;
            case "ButtonEquals":
                buttonEquals = (ButtonEquals)component;
                break;
            case "ButtonAllClear":
                buttonAllClear = (ButtonAllClear)component;
                break;
            case "TextField":
                textField = (TextField)component;
                break;
        }
    }

    @Override
    public void addToScreen(String value) {
        if (this.clearOnNextNumber) {
            replaceOnScreen(value);
            this.clearOnNextNumber = false;
        } else {
            textField.setText(textField.getText() + value);
        }
    }

    @Override
    public void addOperation(int value) {
        this.number1 = textField.getText();
        this.clearScreen();
        this.operation = value;
        this.clearOnNextNumber = true;
        textField.setText(this.number1);
    }

    @Override
    public void addDot() {
        if (!textField.getText().contains(".")) {
            if (textField.getText().equals("")) {
                this.addToScreen("0");

            }
            this.addToScreen(".");
        }
    }

    @Override
    public void replaceOnScreen(String value) {
        // TODO: add flashing
        textField.setText(value);
    }

    @Override
    public void clearOne() {
        textField.setText( Utils.removeLastChar( textField.getText() ) );
    }

    @Override
    public void clearScreen() {
        textField.setText("");
    }

    @Override
    public void clearAll() {
        textField.setText("");
        this.memory = "";
        this.number1 = "";
        this.number2 = "";
        this.operation = 0;
    }

    @Override
    public void calculate() {
        if (this.operation != 0) {
            if (this.number1 == null) {
                this.number1 = textField.getText();
            } else {
                this.number2 = textField.getText();
            }
            String result = Calculator.calculate(Double.parseDouble(this.number1), Double.parseDouble(this.number2), this.operation);
            // TODO: add flashing
            textField.setText(result);
            this.number1 = null;
        }
    }

    @Override
    public void createGUI() {
        JFrame calculator = new JFrame("Calculator");
        calculator.setSize(200, 330);
        calculator.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);

        JPanel mainPanel = new JPanel();
        mainPanel.setLayout(new BoxLayout(mainPanel, BoxLayout.Y_AXIS));

        JPanel input = new JPanel();
        input.setMaximumSize(new Dimension(200, 70));
        input.setLayout(new BoxLayout(input, BoxLayout.LINE_AXIS));

        textField = new TextField();
        Font font = new Font("SansSerif", Font.PLAIN, 24);
        textField.setFont(font);
        textField.setHorizontalAlignment(JTextField.RIGHT);
        textField.setEditable(false);
        textField.setBounds(30, 40, 200, 70);
        input.add(textField);

        JPanel buttons = new JPanel(new GridBagLayout());
        GridBagConstraints gridConstraints = new GridBagConstraints();
        buttons.setSize(200, 260);

        // Grid Bag default
        gridConstraints.fill = GridBagConstraints.BOTH;
        gridConstraints.weightx = 0.5;
        gridConstraints.weighty = 0.5;

        gridConstraints.gridx = 0;
        gridConstraints.gridy = 0;
        buttons.add(buttonAllClear, gridConstraints);

        gridConstraints.gridx = 1;
        gridConstraints.gridy = 0;
        buttons.add(buttonDivide, gridConstraints);

        gridConstraints.gridx = 2;
        gridConstraints.gridy = 0;
        buttons.add(buttonMultiply, gridConstraints);

        gridConstraints.gridx = 3;
        gridConstraints.gridy = 0;
        buttons.add(buttonMinus, gridConstraints);

        gridConstraints.gridx = 0;
        gridConstraints.gridy = 1;
        buttons.add(buttonOne, gridConstraints);

        gridConstraints.gridx = 1;
        gridConstraints.gridy = 1;
        buttons.add(buttonTwo, gridConstraints);

        gridConstraints.gridx = 2;
        gridConstraints.gridy = 1;
        buttons.add(buttonThree, gridConstraints);

        gridConstraints.gridx = 3;
        gridConstraints.gridy = 1;
        gridConstraints.gridheight = 2;
        buttons.add(buttonPlus, gridConstraints);
        // reset
        gridConstraints.gridheight = 1;

        gridConstraints.gridx = 0;
        gridConstraints.gridy = 2;
        buttons.add(buttonFour, gridConstraints);

        gridConstraints.gridx = 1;
        gridConstraints.gridy = 2;
        buttons.add(buttonFive, gridConstraints);

        gridConstraints.gridx = 2;
        gridConstraints.gridy = 2;
        buttons.add(buttonSix, gridConstraints);

        gridConstraints.gridx = 3;
        gridConstraints.gridy = 3;
        gridConstraints.gridheight = 2;
        buttons.add(buttonEquals, gridConstraints);
        // reset
        gridConstraints.gridheight = 1;

        gridConstraints.gridx = 0;
        gridConstraints.gridy = 3;
        buttons.add(buttonSeven, gridConstraints);

        gridConstraints.gridx = 1;
        gridConstraints.gridy = 3;
        buttons.add(buttonEight, gridConstraints);

        gridConstraints.gridx = 2;
        gridConstraints.gridy = 3;
        buttons.add(buttonNine, gridConstraints);

        gridConstraints.gridx = 0;
        gridConstraints.gridy = 4;
        buttons.add(buttonDel, gridConstraints);

        gridConstraints.gridx = 1;
        gridConstraints.gridy = 4;
        buttons.add(buttonZero, gridConstraints);

        gridConstraints.gridx = 2;
        gridConstraints.gridy = 4;
        buttons.add(buttonDot, gridConstraints);

        mainPanel.add(input);
        mainPanel.add(buttons);
        calculator.setLayout(null);
        calculator.setContentPane(mainPanel);
        calculator.setResizable(false);
        calculator.setLocationRelativeTo(null);
        calculator.setVisible(true);
    }
}
\$\endgroup\$
0
\$\begingroup\$

First of all, it's nice that you've split your logic among different classes and tried to make them small. And also it's good that you've introduced some interfaces.

Now to the tips: Your mediator is risky. As a user, I can destroy your mediator if I:

  • forget to add all necessary components
  • forget to call "createGUI"
  • call one of the other methods

If you design your classes, you should do everything possible to make them hard to break and easy to use. Imagine you're the user. How would you like to use the class? Personally, I would find the following reasonable:

public static void main(String[] args) {
  new CalculatorWindow(
    new Calculator()
  ).show();
}

It may be that they need some more arguments (window size for example), but at first, this should be enough.

The next is about splitting the jobs:

Calculator: Should be calculating your stuff, but additionally, I would put the clearing logic and similar stuff inside of it. Just let the GUI code out of the calculator. He shouldn't even know that there is a GUI at all.

CalculatorWindow: This class would be similar to your mediator class but without the clearing logic and with some changes:
1) Let it create your buttons. This way you can remove your "registerComponent" method.
2) Remove all other methods besides createGUI. Currently, your buttons call methods like "calculate" on Mediator and thus the Mediator needs these methods. The problem with these methods is that the user can also see and use them to destroy your calculator. There is an alternative:

// when you register a component, instead of:
component.setMediator(this);

// use this:
component.setMediator(
  new MediatorInterface() {
    @Override
    public void clearOne() {
      // ...
    }

    // ...
  }
);

And by the way: If you have an interface like MediatorInterface, use it. Currently, your buttons take the Mediator class in their "setMediator" method.

Button: It's ok to use your own classes that wrap the standard GUI classes but you don't need one for every possible text they are displaying. In your case, one button is enough:

// I wouldn't extend JButton because JButton just fills your interface with
// maaaaany methods.
public final class Button implements Addable {
  private final JButton button; // instead of extending JButton, just keep an instance of it

  public Button(String text, ActionListener actionListener, /* what ever you might want */) {
    button = new JButton(text); 
    // button setting...
  }


  // from Addable:
  @Override
  public void addOn(Container container) {
    container.add(button);
  }
}

Why Addable?
You already have a Component interface but the problem with it is that Swing already has a Component class. Using the same name for an interface is not a good idea, so I chose another name. In this case, it has exactly one method: addOn. This is necessary because this version of Button doesn't extend from JButton and this way you can still add it to other Swing components.
The setMediator method is not necessary because you can just take an action listener as an argument for the constructor and do the setting there. It's more stable than introducing a setter for that.
GetName isn't necessary either.

That's all. These tips won't make everything perfect but they should be a good start.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.