3
\$\begingroup\$

I have built a simple calculator using Swing that can perform two operations, addition and subtraction. I would like to extend the operations and add more functionality, such as multiply, divide, square, square root, etc.

I would like to know how I can improve my code in these areas:

  • OOP
  • Java stylings
  • Readability
  • Overall design and organization. Is there an architectural pattern I could be using? Are there design patterns I could be using?

Note: This is my first Java application, coming from JavaScript/ReactJS. All classes have their own file, so I just threw everything into one below. I couldn't include textfield, numbergrid, and operator classes. I was getting a message that said my post contained too much code.

public class Client {
            public static void main(String[] args) {
                new Calculator();
            }
        }

import javax.swing.*;
import java.awt.event.*;
import java.util.ArrayList;
import java.util.List;

public class Calculator implements ActionListener {

    JFrame layout = new JFrame();

    private JTextArea display;
    private List<String> input = new ArrayList<>();

    Calculator() {

        new TextField(this);

        new NumberGrid(this);
        new Operators(this);

        layout.setSize(600,500);//600 width and 500 height
        layout.setLayout(null);//using no layout managers
        layout.setVisible(true);//making the frame visible
    }

    @Override
    public void actionPerformed(ActionEvent e) {
        JButton buttonCalled = (JButton) e.getSource();
        String buttonValue = buttonCalled.getText();

        input.add(buttonValue);
        display.append(buttonValue);

        if (buttonValue.equals("=")) {
            System.out.print("Checking for equals");
            getResult();
        }
    }

    void getResult() {

        //loop through elements in ArrayList
        //perform operations
        String x = "";
        String y = "";

        String operation = "";
        int finalResult = 0;

        Boolean isX = true;

        for (int i = 0; i < input.size(); i++) {
            //check if it is a number or operations
            if (isNumeric(input.get(i)) && isX) {
                x += input.get(i);
            }

            if (input.get(i).equals("+") || input.get(i).equals("-")) {
                operation = input.get(i);
                isX = false;
                continue;
            }

            if (isNumeric(input.get(i)) && !isX) {
                y += input.get(i);
            }
        }

        clearInputField(display);

        switch (operation) {
            case "+":
                finalResult = Integer.parseInt(x) + Integer.parseInt(y);
                break;
            case "-":
                finalResult = Integer.parseInt(x) - Integer.parseInt(y);
                break;
        }

        showResult(display, Integer.toString(finalResult));

        input.clear();
        input.add(Integer.toString(finalResult));
    }

    void clearInputField(JTextArea field) {
        field.setText("");
    }

    void showResult(JTextArea field, String result) {
        field.setText(result);
    }

    public boolean isNumeric(String str) {
        try
        {
            double d = Double.parseDouble(str);
        }
        catch(NumberFormatException nfe)
        {
            return false;
        }
        return true;
    }

}

import javax.swing.*;

public class TextField {

    TextField (Calculator calculator) {
        JTextArea inputField = new JTextArea();
        inputField.setBounds(130, 50, 200, 15);
        calculator.layout.add(inputField);
    }
}

import javax.swing.*;
import java.util.ArrayList;

public class Operators {

    private ArrayList<JButton> buttonGroup = new ArrayList<JButton>();

    Operators(Calculator calculator) {
        buildOperator(calculator, '+', 435, 130, 40, 75);
        buildOperator(calculator, '-', 435, 220, 40, 75);
        buildOperator(calculator, '=', 330, 250, 100, 40);
    }

    void buildOperator(Calculator calculator, char operator, int x, int y, int width, int height) {

        JButton newButton = new JButton(Character.toString(operator));
        newButton.setBounds(x, y, width, height);
        newButton.setName(Character.toString(operator));
        newButton.addActionListener(calculator);
        buttonGroup.add(newButton);
        calculator.layout.add(newButton);

    }
}

import javax.swing.*;
import java.util.ArrayList;

public class NumberGrid {

    private int numberOfRows = 3;
    private ArrayList<JButton> buttonGroup = new ArrayList<JButton>();


    NumberGrid(Calculator calculator) {
        buildRows(calculator);
        buildZero(calculator);
    }

    void buildRows(Calculator calculator) {

        int buttonWidth = 100;
        int buttonHeight = 40;

        int buttonXLocation = 130;
        int buttonYLocation = 130;

        int buttonXoffSet = 100;
        int buttonYoffSet = 40;

        int counter = 1;

        for (int col = 1 ; col <=numberOfRows; col++) {

            for (int i = 1; i <= 3; i++) {
                String buttonNumber = Integer.toString(counter);

                buildButton(calculator,
                            buttonNumber,
                            buttonXLocation,
                            buttonYLocation,
                            buttonWidth,
                            buttonHeight);

                buttonXLocation += buttonXoffSet;
                counter++;
            }

            buttonXLocation = 130;
            buttonYLocation += buttonYoffSet;
        }
    }

    void buildButton(Calculator calculator, String buttonNumber, int x, int y, int width, int height) {

        JButton newButton = new JButton(buttonNumber);
        newButton.setBounds(x, y, width, height);
        newButton.setName(buttonNumber);
        newButton.addActionListener(calculator);
        buttonGroup.add(newButton);
        calculator.layout.add(newButton);

    }

    void buildZero(Calculator calculator) {

        int buttonWidth = 200;
        int buttonHeight = 40;

        int buttonXLocation = 130;
        int buttonYLocation = 250;

        String buttonNumber = Integer.toString(0);

        buildButton(calculator, buttonNumber, buttonXLocation, buttonYLocation, buttonWidth, buttonHeight);

    }

}
\$\endgroup\$
3
\$\begingroup\$

Thanks for sharing your code!

You might find this answer worth reading since it also is about a calculator implementation.

In addition to that some findings about your code:

Don't implement *Listener interfaces on class level

Your class Calculator directly implements the ActionListener interface. This leads to a rather procedural approach later on because you have to find our which button was pressed. You work around this problem by delaying the decision until the switch statement in the getResult() method. This way you will have to change multiple places when adding a new function to the calculator.

When implementing the ActionListener interface as separate anonymous inner class for each function in conjunction with the CalculatorFunction interface from my other answer you'll have a single point in your program where you add new calculator functions.

Use LayoutManagers instead of null-layout

You use the null - layout and define the positions of your GUI elements yourself.

You will find that it is quite cumbersome to manage element positions this way as your application grows. Also you completely give away resizability of your GUI. Go through the Layout tutorial: https://docs.oracle.com/javase/tutorial/uiswing/layout/visual.html

Don't directly access variables in other classes/objects

You pass around an instance of the Calculator class so that other classes can access the public field layout. (technically it is *package private, but your intention is clear).

This violates the most important OO principle: information hiding / encapsulation!

No other class should have (direct) access to any field of a class (not even through getter/setter methods).
With the exception of Data Transfer Objects (DTOs) / ValueObjects
You would rather pass the JFrame instance itself around.

Keep same level of abstraction

Your methods do both: calling other methods and "primitive" calculations. But a method should only do either one.

\$\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.