Java calculator using Swing

Question

This is a simple calculator where the user can type out the calculation and hit enter, and the calculator would determine if it is a valid calculation or not by attempting to carry out the calculation. If it fails, then an error message is printed to the screen. If it is a valid calculation, the calculation is carried out.

1. Is the code well written?
2. Is it easily extendable i.e. implementing a button that switches between normal operator panel scientific one?
3. Are there any problems with coupling and cohesion?
4. What could be done to make the code more robust?

Calculator engine:

public class Calculator {
    @SuppressWarnings("unused")
    private CalculatorGui gui;


    private static final int BRACKETS = 5;
    @SuppressWarnings("unused")
    private static final int ORDERS = 4; //for future expansion;
    private static final int DIVISION = 3;
    private static final int MULTIPLICATION = 2;
    private static final int ADDITION = 1;
    private static final int SUBTRACTION = 0;


    public Calculator() {
        this.gui = new CalculatorGui(this);
    }


    public String getAnswer(String text){
        try {

            if(isSingleCalc(text)) {
                return singleCalc(getFirstNum(text), getOperator(text), getSecondNum(text));
            }else {
                String[] nextAndIndexs = getNext(text);
                nextAndIndexs[0] = getAnswer(nextAndIndexs[0]);
                if(nextAndIndexs[0].equals("Invalid Calc")) {
                    return "Invalid Calc";
                }
                String step = insert(text,nextAndIndexs);
                return getAnswer(step);
            }
        } catch (Exception e) {
            return "Invalid Calc";
        }
    }

    private boolean isSingleCalc(String text) {
        boolean singleCalc = true;
        try {
            singleCalc(getFirstNum(text), getOperator(text), getSecondNum(text));
        } catch(Exception e) {
            singleCalc = false;
        }
        return singleCalc;
    }


    private String singleCalc(String firstNum, String operator, String secondNum) throws Exception {
        float ans = 0f;

        switch(operator) {
            case "+": ans = Float.parseFloat(firstNum) + Float.parseFloat(secondNum);
                break;
            case "-": ans = Float.parseFloat(firstNum) - Float.parseFloat(secondNum);
                break;
            case "*": ans = Float.parseFloat(firstNum) * Float.parseFloat(secondNum);
                break;
            case "/": ans = Float.parseFloat(firstNum) / Float.parseFloat(secondNum);
                break;
            default: throw new Exception();     
        }

        return "" + ans;
    }


    private String[] getNext(String text) {
        String currentHighestOper ="";  
        int beginingIndex,endIndex, index;
        beginingIndex = endIndex = index = 0;
        for(int i = 0; i < text.length(); i++) {
            if(isHigherPrecedence("" + text.charAt(i), currentHighestOper)) {
                currentHighestOper = "" + text.charAt(i);
                index = i;  
            }
        }

        if(text.charAt(index) != '(') {
            beginingIndex = getBeginingIndex(index, text);
            endIndex = getEndIndex(index, text);
        } else {
            beginingIndex = index + 1;
            endIndex = getEndBracketIndex(index, text);
        }

        String[] next = {text.substring(beginingIndex, endIndex),"" + beginingIndex,"" + endIndex, "" + text.charAt(index)};

        return next;
    }

    private int getBeginingIndex(int i, String text) {
        for(int j = i-1; j >= 0; j--) {
            if (isOperator("" +text.charAt(j)) && !isNegation(text,j)) {
                return j + 1;
            }
        }
        return 0;
    }


    private int getEndIndex(int i, String text) {
        for(int j = i + 1; j < text.length(); j++) {
            if (isOperator("" +text.charAt(j)) && !isNegation(text, j)) {
                return j;
            }       
        }
        return text.length();
    }


    private int getEndBracketIndex(int i, String text) {
        for(int j = i; j < text.length(); j++) {
            if(text.charAt(j) == ')') {
                return j;
            }
        }
        return text.length() - 1;
    }


    private boolean isOperator(String text) {
        if(text.charAt(0) == '+' ||
                text.charAt(0) == '-' ||
                text.charAt(0) == '*' ||
                text.charAt(0) == '/') {
            return true;
        } else {
            return false;
        }

    }


    private boolean isNegation(String text, int j) {
        if((j==0 || isOperator("" +text.charAt(j-1)) || isOperator("" +text.charAt(j-1))) && text.charAt(j) == '-') {
            return true;
        }else {
            return false;
        }
    }


    private boolean isHigherPrecedence(String oper, String highest) {
        int operValue = getPrecedenceValue(oper);
        int highestValue = getPrecedenceValue(highest);

        return (operValue >= highestValue)? true: false;
    }


    private int getPrecedenceValue(String oper) {
        switch(oper) {
            case "(": return BRACKETS;
            case "/": return DIVISION;
            case "*": return MULTIPLICATION;
            case "+": return ADDITION;
            case "-": return SUBTRACTION;
            default: return -1;
        }
    }

    private String insert(String mainAnswer,String[] nextAndIndexs) {
        StringBuilder sb = new StringBuilder(mainAnswer);

        if(!nextAndIndexs[3].equals("(")) {
            return sb.replace(Integer.parseInt(nextAndIndexs[1]),Integer.parseInt(nextAndIndexs[2]), nextAndIndexs[0]).toString();
        }else {
            return sb.replace(Integer.parseInt(nextAndIndexs[1]) - 1,Integer.parseInt(nextAndIndexs[2]) + 1, nextAndIndexs[0]).toString();
        }


    }


    private String getFirstNum(String text) {
        String firstNum = null;
        for(int i =0; i <text.length(); i++) {
            if(isOperator("" + text.charAt(i)) && !isNegation(text,i)) {
                firstNum = text.substring(0, i);
            }
        }
        return firstNum;
    }

    private String getOperator(String text) {
        String operator = null;
        for(int i =0; i <text.length(); i++) {
            if(isOperator("" + text.charAt(i))&& !isNegation(text,i)) {
                operator = text.substring(i, i+1);
            }
        }
        return operator;
    }

    private String getSecondNum(String text) {
        String secondNum = null;
        for(int i =0; i <text.length(); i++) {
            if(isOperator("" + text.charAt(i))&& !isNegation(text,i)) {
                secondNum = text.substring(i + 1, text.length());
            }
        }
        return secondNum;
    }

    public static void main(String[] args) {
        @SuppressWarnings("unused")
        Calculator calc = new Calculator();
    }

}

GUI:

public interface CalculatorGUIInterface {

    public void writeToScreen(String text);
    public void clearScreen();
    public String getScreenText();
    public String getAnswer();

}

public interface CalculatorInterface {
    public void writeToScreen(String text);
    public void clearScreen();
    public String getScreenText();
    public String getAnswer();
}

public interface CalculatorNumPanelInterface {
    public void writeToScreen(String text);
    public void clearScreen();
    public String getScreenText();  
}

public interface CalculatorOperPanelInterface {
    public void writeToScreen(String text);
    public void clearScreen();
    public String getScreenText();
    public String getAnswer();
}

import java.awt.Color;
import java.awt.ComponentOrientation;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.GridLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.BoxLayout;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.border.EmptyBorder;


@SuppressWarnings("serial")
public class CalculatorGui extends JFrame implements CalculatorGUIInterface {

    private class CalculatorPanel extends JPanel implements CalculatorInterface {

        private class NumberPanel extends JPanel implements CalculatorNumPanelInterface {

            private static final int NUMTOTAL = 10;

            private CalculatorPanel calcPanel;
            private JButton[] numButtons;

            public NumberPanel(CalculatorPanel calcPanel) {
                this.calcPanel = calcPanel;
                buildLayout();
                addButtons();
            }
            private void buildLayout() {
                this.setBorder(new EmptyBorder(0,0,0,5));
                this.setComponentOrientation(ComponentOrientation.RIGHT_TO_LEFT);
                GridLayout layout = new GridLayout(4,3);
                layout.setHgap(1);
                layout.setVgap(1);

                this.setLayout(new GridLayout(4,3));

            }
            private void addButtons() {
                numButtons = new JButton[NUMTOTAL];

                for(int i = numButtons.length -1; i >= 0 ; i--) {
                    numButtons[i] = new JButton("" + i);
                    numButtons[i].setPreferredSize(new Dimension(60,40));
                    numButtons[i].setFont(new Font("Sans serif", Font.PLAIN, 18));
                    numButtons[i].addActionListener(
                            new ActionListener() {
                                @Override
                                public void actionPerformed(ActionEvent e) {
                                    String text = ((JButton)e.getSource()).getText().trim();
                                    if(getScreenText().equals("Invalid Calc")) {
                                        clearScreen();
                                        writeToScreen(text);
                                    }else {
                                        writeToScreen(text);
                                    }       
                                }
                            });
                    this.add(numButtons[i]);
                }
            }
            @Override
            public void writeToScreen(String text) {
                calcPanel.writeToScreen(text);
            }
            @Override
            public void clearScreen() {
                calcPanel.clearScreen();

            }
            @Override
            public String getScreenText() {
                return calcPanel.getScreenText();
            }

        }

        private class OperPanel extends JPanel implements CalculatorOperPanelInterface {

            private static final int ADD = 0;
            private static final int SUB = 1;
            private static final int MULT = 2;
            private static final int DIV = 3;
            private static final int OPENB = 4;
            private static final int CLOSEB = 5;
            private static final int CLEAR = 6;
            private static final int EQL = 7;


            private static final int OPERTOTAL = 8;

            private CalculatorPanel calcPanel;
            private JButton[] operButtons;

            public OperPanel(CalculatorPanel calcPanel) {
                this.calcPanel = calcPanel;
                buildLayout();
                addButtons();
            }           
            private void buildLayout() {
                GridLayout layout = new GridLayout(4,1);
                layout.setHgap(1);
                layout.setVgap(1);

                this.setLayout(new GridLayout(4,1));
            }           
            private void addButtons() {
                operButtons = new JButton[OPERTOTAL];

                operButtons[ADD] = makeButton(ADD, "+");
                operButtons[SUB] = makeButton(SUB, "-");
                operButtons[MULT] = makeButton(MULT, "*");
                operButtons[DIV] = makeButton(DIV, "/");
                operButtons[CLEAR] = makeButton(CLEAR, "CL");
                operButtons[EQL] = makeButton(EQL, "=");
                operButtons[OPENB] = makeButton(OPENB, "(");
                operButtons[CLOSEB] = makeButton(CLOSEB, ")");

                for(JButton button: operButtons) {
                    this.add(button);
                }   
            }           
            private JButton makeButton(int index, String label) {   

                operButtons[index] = new JButton(label);
                operButtons[index].addActionListener(
                        new ActionListener() {
                            @Override
                            public void actionPerformed(ActionEvent e) {
                                String screenText = "";
                                String text = ((JButton)e.getSource()).getText();
                                if(text.equals("=")) {
                                    screenText = getAnswer();
                                    clearScreen();
                                    writeToScreen(screenText);
                                }else if(text.equals("CL")) {
                                    clearScreen();  
                                }else {
                                    writeToScreen(text);
                                }
                            }       
                        });

                return operButtons[index];      
            }   
            @Override
            public String getScreenText() {
                return calcPanel.getScreenText();
            }
            @Override
            public void clearScreen() {
                calcPanel.clearScreen();

            }
            @Override
            public void writeToScreen(String text) {
                calcPanel.writeToScreen(text);

            }
            @Override
            public String getAnswer() {
                return calcPanel.getAnswer();
            }

        }


        private NumberPanel numPanel;
        private OperPanel operPanel;

        private CalculatorGui gui;

        public CalculatorPanel(CalculatorGui gui) {
            this.gui = gui;

            buildNumPanel();
            buildOperPanel();

            buildCalcPanel();


        }
        private void buildNumPanel() {
            this.numPanel = new NumberPanel(this);
        }

        private void buildOperPanel() {
            this.operPanel = new OperPanel(this);
        }

        private void buildCalcPanel() {
            this.setBorder(new EmptyBorder(10,0,0,0));
            this.setLayout(new BoxLayout(this, BoxLayout.X_AXIS));
            this.add(numPanel);
            this.add(operPanel);
        }


        @Override
        public void writeToScreen(String text) {
            gui.writeToScreen(text);    
        }

        @Override
        public String getScreenText() {
            return gui.getScreenText();
        }

        @Override
        public void clearScreen() {
            gui.clearScreen();
        }
        @Override
        public String getAnswer() {
            return gui.getAnswer();
        }
    }
    private Calculator calc;

    private JPanel mainPanel;// used to add padding around Calculator Panel

    private JTextField calcScreen;
    private CalculatorPanel calcPanel;

    public CalculatorGui(Calculator calc) {
        this.calc = calc;

        buildScreen();

        buildCalcPanel();

        buildMainPanel();

        buildCalculator();
    }



    private void buildScreen() {
        this.calcScreen = new JTextField();
        this.calcScreen.setEditable(false);
        this.calcScreen.setBackground(Color.WHITE);
        this.calcScreen.setPreferredSize(new Dimension(150,50));
        this.calcScreen.setHorizontalAlignment(JTextField.CENTER);
        this.calcScreen.setFont(new Font("Sans serif", Font.PLAIN, 30));

    }

    private void buildCalcPanel() {
        this.calcPanel = new CalculatorPanel(this);
    }

    private void buildMainPanel() {
        this.mainPanel = new JPanel();
        this.mainPanel.setBorder(new EmptyBorder(10,10,10,10));
        this.mainPanel.setLayout(new BoxLayout(this.mainPanel, BoxLayout.Y_AXIS));

        this.mainPanel.add(calcScreen);
        this.mainPanel.add(calcPanel);
    }

    private void buildCalculator() {

        this.add(mainPanel);
        this.setTitle("Calculator");
        this.pack();
        this.setResizable(false);
        this.setLocationRelativeTo(null);
        this.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        this.setVisible(true);
    }

    @Override
    public String getAnswer() {     
        return calc.getAnswer(calcScreen.getText());
    }

    @Override
    public void writeToScreen(String text) {
        calcScreen.setText(getScreenText() + text);     
    }

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

    @Override
    public String getScreenText() {
        return calcScreen.getText();
    }
}

Answer 1

Please, use your IDE to format the code.

Calculator

@SuppressWarnings("unused")
private CalculatorGui gui;

If it's unused, then it's useless. Just drop it:

public Calculator() {
    new CalculatorGui(this);
}

Simply creating the GUI without any assignment is fine. It doesn't get GC'd as it's registered internally somewhere in Swing.

But you've got this small part backwards. The GUI needs the engine, not the other way round. Getting rid of the GUI would allow you to write tests without any frames popping up.

---

public String getAnswer(String text){
    try {
        if(isSingleCalc(text)) {

Solving the simple case specially is quite often not needed. And often it's inefficient. You call singleCalc inside of isSingleCalc, throw away the result and call it again to get the result. A better pattern is something like

 try {
     return singleCalc(text);
 } catch (RuntimeException ignored) {
 }
 ... do it the hard way

If you want to avoid exception, then returning null can help

 String result = singleCalc(text);
 if (result != null) {
     return result;
 }
 ... do it the hard way

But usually just

 ... do it the hard way

is strictly better.

---

    } catch(Exception e) {

Don't catch Exception when something more specific suffices (RuntimeException).

Don't make your control flow that much exception driven. Often a simple test suffices.

---

    float ans = 0f;

Never use float unless

• you have to save memory and it's in an array
• you need it for speed and you know it really helps.

Float is damn imprecise and there's no reason for it here. Use double.

---

    switch(operator) {
        case "+": ans = Float.parseFloat(firstNum) + Float.parseFloat(secondNum);
            break;

You could use

double first = Double.parse(firstNum);
double second = Double.parse(secondNum);
    switch(operator) {
        case "+": return first + second;
        case "-": return first - second;

to gain some readability.

        default: throw new Exception();     

This should be

        default: throw new IllegalArgumentException();

possible including an error message.

---

private String[] getNext(String text) {

This doesn't look like returning some "next". It seems to "split" or "parse" the string somehow, so call it accordingly.

    String currentHighestOper ="";  

Use char where it suffices.

    int beginingIndex,endIndex, index;
    beginingIndex = endIndex = index = 0;

Don't predeclare your variables before you need them. Declare one variable per line like

    int beginingIndex = 0;
    int endIndex = 0;
    int index = 0;

and do not initialize variable which don't need it. That's Java, the compiler doesn't let you forget it.

The later part should look like

    int beginingIndex;
    int endIndex;
    if (text.charAt(index) != '(') {
        beginingIndex = getBeginingIndex(index, text);
        endIndex = getEndIndex(index, text);
    } else {
        beginingIndex = index + 1;
        endIndex = getEndBracketIndex(index, text);
    }

---

    String[] next = {text.substring(beginingIndex, endIndex),"" + beginingIndex,"" + endIndex, "" + text.charAt(index)};

That's perverted. You're using a String[] to pass integers. That's what classes are for:

 private static final class ParsedPart {
      String substring;
      int beginingIndex;
      int endIndex;
      char operator;
 }

---

private boolean isOperator(String text) {
    if(text.charAt(0) == '+' ||
            text.charAt(0) == '-' ||
            text.charAt(0) == '*' ||
            text.charAt(0) == '/') {
        return true;
    } else {
        return false;
    }

}

This could be written as

private boolean isOperator(String text) {
    return text.charAt(0) == '+' ||
            text.charAt(0) == '-' ||
            text.charAt(0) == '*' ||
            text.charAt(0) == '/');
}

but actually should be just

private boolean isOperator(char text) {
    return text == '+' ||
            text == '-' ||
            text == '*' ||
            text == '/');
}

unless you mean startsWithOperator.

---

private boolean isNegation(String text, int j) {
    if((j==0 || isOperator("" +text.charAt(j-1)) || isOperator("" +text.charAt(j-1))) && text.charAt(j) == '-') {
        return true;
    }else {
        return false;
    }
}

While j is a cool name for a loop variable, I'd avoid it here. The code is equivalent to

private boolean isNegation(String text, int index) {
    return (index==0 || isOperator(text.charAt(index-1)))
       && text.charAt(index) == '-';
}

as you repeated one term exactly.

---

In getFirstNum, getOperator, getSecondNum, you're doing about the same stuff. Actually, you should probably break after you find the operator. You need to get three things and it looks like you need three methods for it. But you don't. You could

• write a trivial class aggregating firstNum, operator, and secondNum
• or use a String[] (here, it'd alright)
• or use a helper method like findFirstOperatorIndex
• or (preferably) leave this simple case out and rely on the general processing

Summary

It's not bad, just a bit too complicated. I can still recall how hard is it to get one's first parser working. There are tools and there's theory for such stuff, though for a calculator an ad hoc solution is fine. Just make it simpler.

As you can see, I didn't cover everything as you've been given some feedback already. There's surely still a lot more to say. In case you want to work on this code further, I'd suggest to post the engine alone in a followup question (ideally with some JUnit tests).

Answer 2

I recommend creating an enum for your operations for three reasons:

1. The fields you are storing in the beginning (ADDITION, SUBTRACTION...) are structured just like an enum, because an enum orders it's items starting from 0.

2. You can assign the operator symbols to each operation

3. You can assign a method/function to each operator that is called when that operator is needed

---

Here is what the basic enum would look like, going off of the constants you created in the beginning of Calculator:

public enum Operator {
    ADDITION, SUBTRACTION, MULTIPLICATION, DIVISION
}

This, for example, sets the value Operator.MULTIPLICATION to 2.

This is a side note: looking back at your code, I see now why you had those constants: for the order in which they are executed.

Rather than creating an enum for these, I recommend creating an array (or something of that sort) and storing the values from Operator (maybe the int versions of the values) in there, along with the BRACKETS and ORDERS variables that you created.

---

Now, let's add to our enum the operator symbol that goes for each operation. For example, \ goes with the operator DIVISION.

Here is what the enum becomes:

public enum Operator {
    ADDITION('+'), SUBTRACTION('-'),
    MULTIPLICATION('*'), DIVISION('/');

    private final char symbol;
    private Operator(char symbol) {
        this.symbol = symbol;
    }
}

Alright, this was a big addition. Let's break it down:

• What does ADDITION('+') mean?

When I wrote something like ADDITION('+'), this is basically calling the constructor that I made in the below of the enum and passing the character '+' to it.

---

This is all great, but now we should take it a step further: let's implement each operator's function as a lambda, and store this in the enum.

For this, we are going to need to create an interface that our lambdas will be from.

Since all the operator's functions take two parameters and return a single parameter, this makes our interface really easy:

public interface Operation {
    int solve(int a, int b);
}

This might be a little confusing. Keep reading for it to make sense.

Now that we have this interface for our lambdas, we can add our lambdas to the enum:

public enum Operator {
    ADDITION('+', (a, b) -> a + b),
    SUBTRACTION('-', (a, b) -> a - b),
    MULTIPLICATION('*', (a, b) -> a * b),
    DIVISION('/', (a, b) -> a / b);

    private final char symbol;
    private final Operation operation;

    private Operator(char symbol, Operation operation) {
        this.symbol = symbol;
        this.operation = operation;
    }
}

Let's look at a single lambda here: for example, the one for subtraction

(a, b) -> a - b

Does this look familiar? This is the same format as the solve function of the Operation interface: two integers are taken in, and one is returned.

This means that, provided that operation was a public field, we could subtract two parameters by calling

Operator.SUBTRACTION.operation.solve(int a, int b);

This will return a - b.

---

Our enum is looking really good. However, it is still not quite ready for use.

For starters, all the properties are private, so we are going to have to create getters for them. This is relatively easy:

public enum Operator {
    ...
    public char getSymbol() {
        return this.symbol;
    }
    public Operation getOperation() {
        return this.operation;
    }
}

Now we can use, for example

Operator.DIVISION.getSymbol()

And we will get the symbol for division which is '/'.

---

Now our enum is fully usable. However, it's not very helpful to the rest of the code.

Let's say the user hits the + button. Well, then what? How do we get the right enum so we can call the operation method?

We need to add some static functions to our Operator enum so that we can locate certain enum values using an input value, like the operator symbol. And, we can make this easier on ourselves by using the values method of an enum.

To create a method for finding an enum by it's operator symbol, we are going to have to do the following:

1. Get the operator symbol.

2. Loop through all the Operator values.

3. If the current Operator value's symbol is equal to the input symbol, return the current Operator symbol.

This comes out to this in Java:

public enum Operator {
    ...

    public static Operator getOperatorBySymbol(char symbol) {
        for(Operator operator : Operator.values()) {
            if(operator.getSymbol() == symbol) {
                return operator
            }
        }
    }
}

---

This should simplify the rest of your Java code by quite a lot.

Let's say the user enters some mathematical expression. After the calculator has decided the order of operations and all that other stuff, it can simply call a function like this:

public int compute(int a, int b, char operator) {
    return Operator.getOperatorBySymbol(operator).getOperation().solve(a, b);
}

---

Thanks to h.j.k. for this information:

Instead of creating a brand new interface for writing the lambda expression, you can simply use the IntBinaryOperator interface.