Basic MDAS Java Swing Calculator

Question

I have recently started learning Java and decided to make a basic MDAS calculator in Swing. I am not completely new to programming but I may be making some common mistakes, or not writing the most efficient code.

I wanted to make a calculator that can take multiple numbers and operations before finding the answer using MDAS, instead of just returning the answer after every operation and using it for the next.

e.g. 2 * 3 + 4 - 5 / 5 = 9 instead of 1

My code consists of a single class. There isn't much code so I didn't know if there was a good reason to split it into multiple classes, however I have never written something like this so please feel free to correct me.

Repo with example gif and runnable jar

---

package calculator;

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.util.ArrayList;

import javax.swing.AbstractAction;
import javax.swing.BorderFactory;
import javax.swing.Box;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;

public class GUI extends JFrame {
    
    private static final long serialVersionUID = 1L;
    
    private String title = "Basic MDAS Calculator";
    private int currentNumber;
    private JLabel displayLabel = new JLabel(String.valueOf(currentNumber), JLabel.RIGHT);
    private JPanel panel = new JPanel();
    
    private boolean isClear = true;
    
    final String[] ops = new String[] {"+", "-", "x", "/"};
    
    private ArrayList<Integer> numHistory = new ArrayList<Integer>();
    private ArrayList<String> opHistory = new ArrayList<String>();
    

    public GUI() {
        setPanel();
        setFrame();
    }
    
    private void setFrame() {
        this.setTitle(title);
        this.add(panel, BorderLayout.CENTER);
        this.setBounds(10,10,300,700); 
        this.setResizable(false);
        this.setVisible(true);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }
    
    private void setPanel() {
        panel.setBorder(BorderFactory.createEmptyBorder(10, 10, 10, 10));
        panel.setLayout(new GridLayout(0, 1));
        
        displayLabel.setFont(new Font("Verdana", Font.PLAIN, 42));
        panel.add(displayLabel);
        panel.add(Box.createRigidArea(new Dimension(0, 0)));
        
        createButtons();
    }
    
    private void createButtons() {
        
        // 0-9
        for (int i = 0; i < 10; i++) {
            final int num = i;
            
            JButton button = new JButton( new AbstractAction(String.valueOf(i)) { 
                private static final long serialVersionUID = 1L;

                @Override
                public void actionPerformed(ActionEvent e) {
                    
                    // If somebody presses "=" and then types a number, start a new equation instead
                    // of adding that number to the end like usual
                    if (!isClear) {
                        currentNumber = 0;
                        isClear = true;
                    }
                    
                    if (currentNumber == 0) {
                        currentNumber = num;
                    } else {
                        currentNumber = currentNumber * 10 + num;
                    }
                    
                    displayLabel.setText(String.valueOf(currentNumber));
                }
            });
            
            panel.add(button);
        }
        
        // +, -, x, /
        for (String op : ops) {
            
            JButton button = new JButton( new AbstractAction(op) { 
                private static final long serialVersionUID = 1L;

                @Override
                public void actionPerformed(ActionEvent e) {
                    numHistory.add(currentNumber);
                    currentNumber = 0;
                    
                    opHistory.add(op);
                    displayLabel.setText(op);
                }
            });
            
            panel.add(button);
        }
        
        // =
        JButton button = new JButton( new AbstractAction("=") { 
            private static final long serialVersionUID = 1L;
            private int i;

            @Override
            public void actionPerformed(ActionEvent e) {    
                // Display result
                numHistory.add(currentNumber);
                
                while (opHistory.size() > 0) {
                    
                    if (opHistory.contains("x")) {
                        i = opHistory.indexOf("x");
                        numHistory.set(i, numHistory.get(i) * numHistory.get(i+1));
                    } else if (opHistory.contains("/")) {
                        i = opHistory.indexOf("/");
                        numHistory.set(i, numHistory.get(i) / numHistory.get(i+1));
                    } else if (opHistory.contains("+")) {
                        i = opHistory.indexOf("+");
                        numHistory.set(i, numHistory.get(i) + numHistory.get(i+1));
                    } else if (opHistory.contains("-")) {
                        i = opHistory.indexOf("-");
                        numHistory.set(i, numHistory.get(i) - numHistory.get(i+1));
                    }
                    
                    opHistory.remove(i);
                    numHistory.remove(i+1);
                }
                
                displayLabel.setText(String.valueOf(numHistory.get(0)));
                
                currentNumber = numHistory.get(0);
                numHistory.clear();
                
                if (isClear) {
                    isClear = false;
                }
            }
        });
        
        panel.add(button);
    }

    public static void main(String[] args) {
        new GUI();
    }

}

I would appreciate any tips.

Answer 1

package calculator;

Package names should associate the software with the author, like com.github.razemoon.basicmdasjavacaluclator.

---

public class GUI extends JFrame {

For Java nming conventions, you'd normally use UpperCamelCase, and use lowercase even for acronyms, like "Gui", "HtmlWidgetToolkit" or "HtmlCssParser".

---

private static final long serialVersionUID = 1L;

You only need this field if it is highly likely that the class will be serialized...in this case, most likely not.

---

final String[] ops = new String[] {"+", "-", "x", "/"};

Why is this package-private?

Also, final arrays are not as final as you'd think, the individual values can still be changed. You most likely want an Enum....actually, you want an interface, but in this example, an Enum would most likely do fine enough.

---

    private ArrayList<Integer> numHistory = new ArrayList<Integer>();
    private ArrayList<String> opHistory = new ArrayList<String>();

Always try to use the lowest common interface for declarations, in this case List.

---

    private void setFrame() {
        this.setTitle(title);
        this.add(panel, BorderLayout.CENTER);
        this.setBounds(10,10,300,700); 
        this.setResizable(false);
        this.setVisible(true);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

Why are you using this here but nowhere else?

---

        this.setResizable(false);

Why? Your frame is perfectly resizable as far as I can see. By setting it not-resizable you only make sure that your application becomes unusable under different LaFs and font-sizes.

---

for (int i = 0; i < 10; i++) {

I'm a very persistent advocate for that you're only allowed to use single-letter variable names if your dealing with dimensions.

for (int number = 0; number <= 9; number++) {
// Or
for (int digit = 0; digit <= 9; digit++) {

---

final int num = i;

Don't shorten variable names just because you can, the decreased amount of typing is not worth the decreased readability.

---

Regarding the creation of buttons, I like to create helper methods and classes which make the code easier to read, in this case I'd go for lambdas, like this:

private JButton createButton(String text, Runnable action) {
    return new JButton(new AbstractButton(text) {
        @Override
        public void actionPerformed(ActionEvent e) {
            action.run();
        }
    })
}

// In createButtons:

panel.add(createButton(Integer.toString(number), () -> {
    // Code for the number button goes here.
}));

Another alternative would be to create a private class NumberAction which accepts a number in its constructor and performs the associated action. That would also allow you to get rid of the final-redeclaration.

---

private int i;

That;s a very bad variable name.

---

    public GUI() {
        setPanel();
        setFrame();
    }
    
    private void setFrame() {
        this.setTitle(title);
        this.add(panel, BorderLayout.CENTER);
        this.setBounds(10,10,300,700); 
        this.setResizable(false);
        this.setVisible(true);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

// ...

    public static void main(String[] args) {
        new GUI();
    }

It would be better to split the responsibilities here. The frame itself is only responsible with getting its own layout going, while the main method should be responsible for getting the frame displayed.

    public GUI() {
        setPanel();
        setFrame();
    }
    
    private void setFrame() {
        this.setTitle(title);
        this.add(panel, BorderLayout.CENTER);
        this.setBounds(10,10,300,700); 
        this.setResizable(false);
    }

// ...

    public static void main(String[] args) {
        GUI gui = new GUI();
        gui.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        gui.setVisible(true);
    }

---

Your logic does not seem to contain any sort of error handling, I believe pressing an operator button twice in succession should yield an error.

---

Maybe a better approach would be to print the whole expression to the screen as entered, and then apply the Shunting Yard Algorithm to process that expression.

---

Your logic doesn't do decimals, neither does it gracefully handle overflows. By changing your logic yo use BigDecimals you could handle both easily. Note that you must create BigDecimals with an appropriate MathContext to have proper accuracy and behavior.

---

If you want read an already existing implementation, I can recommend exp4j for a math-expression library using floats, EvalEx for one using BigDecimal, and my own project jMathPaper for a calculator which sport different GUIs (regarding abstraction).

Answer 2

You can use the array declaration.

When you want an array with pre-defined values that can be constant, you can declare the array anonymously.

  final String[] ops = {"+", "-", "x", "/"};

Use Enums for the operations.

Instead of having an array of operation, I suggest that you create an Enum instead.

public enum Operators {
   PLUS("+"), MINUS("-"), MUL("x"), DIV("/");
   private final String operator;
   Operators(String operator) {
      this.operator = operator;
   }
   public String getOperator() {
      return operator;
   }
}

This will give you more advantage than the array, since you will be able to remove the duplication.

//[...]
for (Operators op : Operators.values()) {
   JButton button = new JButton( new AbstractAction(op.getOperator()) {
      private static final long serialVersionUID = 1L;

      @Override
      public void actionPerformed(ActionEvent e) {
         numHistory.add(currentNumber);
         currentNumber = 0;

         opHistory.add(op);
         displayLabel.setText(String.valueOf(op.getOperator()));
      }
   });
   panel.add(button);
}
//[...]

//[...]
if (opHistory.contains(Operators.MUL)) {
   i = opHistory.indexOf(Operators.MUL);
   numHistory.set(i, numHistory.get(i) * numHistory.get(i + 1));
} else if (opHistory.contains(Operators.DIV)) {
   i = opHistory.indexOf(Operators.DIV);
   numHistory.set(i, numHistory.get(i) / numHistory.get(i + 1));
} else if (opHistory.contains(Operators.PLUS)) {
   i = opHistory.indexOf(Operators.PLUS);
   numHistory.set(i, numHistory.get(i) + numHistory.get(i + 1));
} else if (opHistory.contains(Operators.MINUS)) {
   i = opHistory.indexOf(Operators.MINUS);
   numHistory.set(i, numHistory.get(i) - numHistory.get(i + 1));
}
//[...]

Also, in my opinion, this will make the code easier to work with and refactor.

When dividing, always check the divisor before doing the division.

When dividing by zero, there an java.lang.ArithmeticException throw by java; I suggest that you add a check :)

Use the Queue instead of the List to keep the history.

By using the List you have to use an index, the Queue to remove the first item (java.util.Queue#poll); the only drawback, you will need to refactor the actual code to remove the indexOf.

private Queue<String> opHistory = new ArrayDeque<>();

By doing so, you will make the code shorter.

while (opHistory.size() > 0) {
   Operators currentOperator = opHistory.poll();

   switch (currentOperator) { //Java 14+ Switch, you can use if or the older version of the switch.
       case MUL -> numHistory.set(i, numHistory.get(i) * numHistory.get(i+1));
       case DIV -> numHistory.set(i, numHistory.get(i) / numHistory.get(i+1));
       case PLUS -> numHistory.set(i, numHistory.get(i) + numHistory.get(i+1));
       case MINUS -> numHistory.set(i, numHistory.get(i) - numHistory.get(i+1));
   }

   numHistory.remove(i + 1);
}

Extract the expression to variables when used multiple times.

In your code, you can extract the similar expressions into variables; this will make the code shorter and easier to read.

final Integer first = numHistory.get(i);
final Integer second = numHistory.get(i + 1);
switch (currentOperator) {
    case MUL -> numHistory.set(i, first * second);
    case DIV -> numHistory.set(i, first / second);
    case PLUS -> numHistory.set(i, first + second);
    case MINUS -> numHistory.set(i, first - second);
}