Java GUI 2-input Calculator

Question

I have written this code for a two input calculator. Is there anything I can do to make this code more efficient?

import javax.swing.*;

import java.awt.*;
import java.awt.event.*;

public class CalcGui extends JFrame {

    private JButton buttonZero;
    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 opButtonPlus;
    private JButton opButtonMinus;
    private JButton opButtonDivide;
    private JButton opButtonMultiply;
    private JButton opButtonEquals;
    private JButton opButtonClear;

    private TextField tf;

    private JPanel numButtonPanel;
    private JPanel opButtonPanel;
    private JPanel basePanel;

    public CalcGui(){
        super("Scientific Calculator");
        basePanel = new JPanel();
        numButtonPanel = new JPanel(new GridLayout(0,4));
        opButtonPanel = new JPanel(new GridLayout(0,1));

        tf = new TextField(20);
        tf.setEditable(false);
        basePanel.add(tf);

         buttonZero = new JButton("0");
         numButtonPanel.add(buttonZero);
         buttonOne = new JButton("1");
         numButtonPanel.add(buttonOne);
         buttonTwo = new JButton("2");
         numButtonPanel.add(buttonTwo);
         buttonThree = new JButton("3");
         numButtonPanel.add(buttonThree);
         buttonFour = new JButton("4");
         numButtonPanel.add(buttonFour);
         buttonFive = new JButton("5");
         numButtonPanel.add(buttonFive);
         buttonSix = new JButton("6");
         numButtonPanel.add(buttonSix);
         buttonSeven = new JButton("7");
         numButtonPanel.add(buttonSeven);
         buttonEight = new JButton("8");
         numButtonPanel.add(buttonEight);
         buttonNine = new JButton("9");
         numButtonPanel.add(buttonNine);

         basePanel.add(numButtonPanel);

        opButtonPlus = new JButton("+");
        opButtonMinus = new JButton("-");
        opButtonDivide = new JButton("/");
        opButtonMultiply = new JButton("*");
        opButtonEquals = new JButton("=");
        opButtonClear = new JButton("C");

        opButtonPanel.add(opButtonPlus);
        opButtonPanel.add(opButtonMinus);
        opButtonPanel.add(opButtonDivide);
        opButtonPanel.add(opButtonMultiply);
        opButtonPanel.add(opButtonEquals);
        opButtonPanel.add(opButtonClear);

        basePanel.add(opButtonPanel);

        add(basePanel);

        HandlerClass handler = new HandlerClass();

        buttonZero.addActionListener(handler);
        buttonOne.addActionListener(handler);
        buttonTwo.addActionListener(handler);
        buttonThree.addActionListener(handler);
        buttonFour.addActionListener(handler);
        buttonFive.addActionListener(handler);
        buttonSix.addActionListener(handler);
        buttonSeven.addActionListener(handler);
        buttonEight.addActionListener(handler);
        buttonNine.addActionListener(handler);

        opButtonPlus.addActionListener(handler);
        opButtonMinus.addActionListener(handler);
        opButtonDivide.addActionListener(handler);
        opButtonMultiply.addActionListener(handler);
        opButtonEquals.addActionListener(handler);
        opButtonClear.addActionListener(handler);

    }


    private class HandlerClass implements ActionListener{
        public void actionPerformed(ActionEvent e){
            String alreadyDisplayed = tf.getText();
            if(e.getSource()==buttonZero){
                String buttonZeroText = alreadyDisplayed + buttonZero.getText();
                tf.setText(buttonZeroText);
            }else if(e.getSource()==buttonOne){
                String buttonOneText = alreadyDisplayed + buttonOne.getText();
                tf.setText(buttonOneText);
            }else if(e.getSource()==buttonTwo){
                String buttonTwoText = alreadyDisplayed + buttonTwo.getText();
                tf.setText(buttonTwoText);
            }else if(e.getSource()==buttonThree){
                String buttonThreeText = alreadyDisplayed + buttonThree.getText();
                tf.setText(buttonThreeText);
            }else if(e.getSource()==buttonFour){
                String buttonFourText = alreadyDisplayed + buttonFour.getText();
                tf.setText(buttonFourText);
            }else if(e.getSource()==buttonFive){
                String buttonFiveText = alreadyDisplayed + buttonFive.getText();
                tf.setText(buttonFiveText);
            }else if(e.getSource()==buttonSix){
                String buttonSixText = alreadyDisplayed + buttonSix.getText();
                tf.setText(buttonSixText);
            }else if(e.getSource()==buttonSeven){
                String buttonSevenText = alreadyDisplayed + buttonSeven.getText();
                tf.setText(buttonSevenText);
            }else if(e.getSource()==buttonEight){
                String buttonEightText = alreadyDisplayed + buttonEight.getText();
                tf.setText(buttonEightText);
            }else if(e.getSource()==buttonNine){
                String buttonNineText = alreadyDisplayed + buttonNine.getText();
                tf.setText(buttonNineText);
            }else if(e.getSource()==opButtonPlus){
                String opButtonPlusText = alreadyDisplayed + opButtonPlus.getText();
                tf.setText(opButtonPlusText);
            }else if(e.getSource()==opButtonEquals){
                String opButtonEqualsText = alreadyDisplayed + opButtonEquals.getText();
                tf.setText(opButtonEqualsText);
            }else if(e.getSource()==opButtonMinus){
                String opButtonMinusText = alreadyDisplayed + opButtonMinus.getText();
                tf.setText(opButtonMinusText);
            }else if(e.getSource()==opButtonMultiply){
                String opButtonMultiplyText = alreadyDisplayed + opButtonMultiply.getText();
                tf.setText(opButtonMultiplyText);
            }else if(e.getSource()==opButtonDivide){
                String opButtonDivideText = alreadyDisplayed + opButtonDivide.getText();
                tf.setText(opButtonDivideText);
            }else if(e.getSource()==opButtonClear){
                String opButtonClearText = "";
                tf.setText(opButtonClearText);
            }

            double result=0.0;
            for(int i=0; i<alreadyDisplayed.length();i++){

                if(e.getSource()==opButtonEquals){

                    if(alreadyDisplayed.charAt(i)=='+'){
                        result = Double.parseDouble(alreadyDisplayed.substring(0,i)) + Double.parseDouble(alreadyDisplayed.substring(i+1,alreadyDisplayed.length()));
                        tf.setText(alreadyDisplayed + "=" + result);
                    }else if(alreadyDisplayed.charAt(i)=='-'){
                        result = Double.parseDouble(alreadyDisplayed.substring(0,i)) - Double.parseDouble(alreadyDisplayed.substring(i+1,alreadyDisplayed.length()));
                        tf.setText(alreadyDisplayed + "=" + result);
                    }else if(alreadyDisplayed.charAt(i)=='/'){
                        result = Double.parseDouble(alreadyDisplayed.substring(0,i)) / Double.parseDouble(alreadyDisplayed.substring(i+1,alreadyDisplayed.length()));
                        tf.setText(alreadyDisplayed + "=" + result);
                    }else if(alreadyDisplayed.charAt(i)=='*'){
                        result = Double.parseDouble(alreadyDisplayed.substring(0,i)) * Double.parseDouble(alreadyDisplayed.substring(i+1,alreadyDisplayed.length()));
                        tf.setText(alreadyDisplayed + "=" + result);
                    }
                }
            }

        }
    }
}

Answer 1

The first (and biggest) recommendation I have is to use a JButton array to store your numeric buttons. This will drastically reduce the amount of duplicate code you have written:

private final List<JButton> numberButtons;
private final static int NUMBER_OF_DIGITS = 10;

Secondly, I would recommend if you consistently name your components. I'm not sure what op stands for, but you can either list the component type first, or list it last, but do it consistently. I've decided to put the component type after:

private final JButton plusButton;
private final JButton minusButton;
private final JButton divideButton;
private final JButton multiplyButton;
private final JButton equalsButton;
private final JButton clearButton;

private final TextField displayField;

private final JPanel numbersPanel;
private final JPanel operatorsPanel;
private final JPanel basePanel;

If you notice in the above code, I've also added final to each of the fields, as you don't plan on changing them.

Now, inside your constructor, I'd recommend getting rid of some magic numbers:

private static final String TITLE = "Scientific Calculator";
private static final int NUMBERS_PER_ROW = 4;
private static final int OPERATORS_PER_ROW = 1;
private static final int OUTPUT_FIELD_WIDTH = 20;

Next, as you are creating your new buttons, I would recommend creating a different ActionListener for each button. This will remove the massive if/else you have in your HandlerClass. Also, you are frequently appending to the text, so using a function for that would be ideal. For example,

for (int i = 0; i < NUMBER_OF_DIGITS; i++){
  final JButton button = new JButton(""+i);
  button.addActionListener(new ActionListener(){
     public void actionPerformed(ActionEvent e){
        addToDisplay(button.getText());
     }
  });
}

with the function:

private void addToDisplay(String text){
    displayField.setText(displayField.getText()+text);
}

Then when you create your operator buttons, you can add the appropriate logic right there, instead of doing it all in the same function.

Answer 2

Your constructor for CalcGui is really long and confusing. Even though you have to do all this GUI creation, you can still group things together to make things more readable.

Also, you should be able to split some things up as different methods that are called by this constructor.

---

This chunk of code is very repetitive:

buttonZero = new JButton("0");
numButtonPanel.add(buttonZero);
buttonOne = new JButton("1");
numButtonPanel.add(buttonOne);
buttonTwo = new JButton("2");
numButtonPanel.add(buttonTwo);
buttonThree = new JButton("3");
numButtonPanel.add(buttonThree);
...

The only difference that is appearing in this code is that every other line, the string passed to JButton is different.

This block is extremely ugly. This can be made look better if you make a loop.

I recommend creating a for-loop that starts at 0 and goes to the maximum value you want on your buttons: you can just use the for loop iterator variable as the value to set the button to.

Here is how I would do that:

for(int i = 0; i < 10; i++) {
    numButtonPanel.add(new JButton(Integer.valueOf(i).toString()));
}

The above code that I wrote does that same exact thing as that big chunk of code that you wrote does, only this is in just a few lines.

I made another subtle change to the code: rather than storing the new JButton in a variable, I passed the JButton directly to newButtonPanel.add.

---

Continuing to look at the rest of your constructor, virtually everything can be simplified with loops.

For example, these lines here:

opButtonPanel.add(opButtonPlus);
opButtonPanel.add(opButtonMinus);
opButtonPanel.add(opButtonDivide);
opButtonPanel.add(opButtonMultiply);
opButtonPanel.add(opButtonEquals);
opButtonPanel.add(opButtonClear);

Can be simplified if you store the buttons in an an array, and then loop through the array as you add them to opButtonPanel.

This looks just like the code I wrote in the last recommendation:

JButton[] opButtons = {opButtonPlus, opButtonMinus...};
for(i = 0; i < opButtons.length; i++) {
    opButtonPanel.add(opButtons[i]);
}

Look into the rest of your constructor: you have quite a lot of lines that are almost exactly the same, except for one thing. You should fix these as I have shown you.

---

Adding all these loops is great because it really increases the readability of the code. However, the constructor is still quite long. Now, we should split the constructor up into separate methods.

For example, when you are setting the Action Listeners, you could split this up into a method and then call it like this:

addButtonActionListeners();

I would make addButtonActionListeners look something like this:

private void addButtonActionListeners() {
    HandlerClass handler = new HandlerClass();
    JButton[] buttons = {buttonZero... opButtonPlus...};

    for(...) {
        buttons[i].addActionListener(handler);
    }
}

Look back at the past snippets if you have trouble filling in the ...s

If you split more chunks of those new for-loops from your constructor into separate methods, your constructor will be reduced to a few, simple, readable lines.

It might look something like this:

public CalcGui() {
    super("Scientific Calculator");
    basePanel = new JPanel();
    numButtonPanel = new JPanel(new GridLayout(0,4));
    opButtonPanel = new JPanel(new GridLayout(0,1));

    tf = new TextField(20);
    tf.setEditable(false);
    basePanel.add(tf);

    makeNumberButtons();
    makeOpButtons();
    addOpButtonsToPanel();
    addButtonActionListeners();
}

---

Now let's look at the HandlerClass class.

The first thing I notice is that giant for-loop. Again, every single case of the for-loop looks almost identical, except for the button being acted upon. Therefore, I would normally recommend that you create a for-loop for this.

However, because a user can only click those buttons on your calculator, I'd say that any sort of for-loop is completely irrational; you already have the button that was clicked in e.getSource(). Therefore, why don't you use that?

Now the code becomes really simple:

String alreadyDisplayed = tf.getText();
if(e.getSource() == opButtonEquals) {
    ...
} else if (e.getSource() == opButtonClear) {
    tf.setText("");
} else {
    tf.setText(alreadyDisplayed + e.getSource().getText());
}

See how your big if/else if was reduced to this? I figured that, since e.getSource() return the button that was clicked, you could just access the button's properties from that.

Then, I saw that opButtonClear and opButtonEquals had special cases so I created specific if/else if cases for them.

I also made one more improvement: rather than storing

alreadyDisplayed + e.getSource().getText();

in it's own variable, I just directly sent the value over to tf.setText.

---

Now that there is a conditional for opButtonEquals in the new if/else structure that I created, there is no need to have that conditional in the for loop.

---

Looking at the for loop, it would be a lot more clean if, instead of using an if/else if statement, you used a switch statement.

That would look like this:

switch(alreadyDisplayed.charAt(i)) {
    case '+':
        ...
        break;
    case '-':
        ...
        break;
    case '*':
        ...
        break;
    case '/':
        ...
        break;
}

Answer 3

I suggest you should collect the number buttons in a Collection<JButton>

This allows you to replace the if chain:

if(e.getSource()==buttonZero){
    String buttonZeroText = alreadyDisplayed + buttonZero.getText();
    tf.setText(buttonZeroText);
}else //...

with

if(numberButtons.contains(e.getSource()){
    JButton number = (JButton)e.getSource();
    String buttonText = alreadyDisplayed + number.getText();
    tf.setText(buttonText);
}

You should separate out the different handlers, make one for the number buttons, one for the operators, one for the equals and one for the clear. That way you don't need to test which button is pressed as much because you know the code wouldn't be executed otherwise.

Also I suggest storing the first and second number as separate numbers instead of collecting the entire string and then parsing it.

Answer 4

This huge chunk of code here:

    buttonZero.addActionListener(handler);
    buttonOne.addActionListener(handler);
    buttonTwo.addActionListener(handler);
    buttonThree.addActionListener(handler);
    buttonFour.addActionListener(handler);
    buttonFive.addActionListener(handler);
    buttonSix.addActionListener(handler);
    buttonSeven.addActionListener(handler);
    buttonEight.addActionListener(handler);
    buttonNine.addActionListener(handler);

    opButtonPlus.addActionListener(handler);
    opButtonMinus.addActionListener(handler);
    opButtonDivide.addActionListener(handler);
    opButtonMultiply.addActionListener(handler);
    opButtonEquals.addActionListener(handler);
    opButtonClear.addActionListener(handler);

Can be simplified by writing an unlimited argument method that adds the handler, something along the lines of:

private static void bindHandler(HandlerClass handler, JButton... buttons) {
  for (JButton button : buttons) {
    button.addActionListener(handler);
  }
}

which would you use by calling:

bindHandler(handler, buttonZero, buttonOne, buttonTwo,/*...*/ opButtonClear);

I'd suggest for succinctness you alternate between using this for the operator buttons, and loop for the numbers.

JButton[] numbers = new JButton[10];
for (int i = 0; i < numbers.length; i++) {
  numbers[i] = new JButton(Integer.toString(i));
  numberPanel.add(numbers[i]);
  numbers[i].addActionListener(handler);
}

I'd probably add spaces since that is fairly long otherwise.

For your division button, you'd probably rather represent it with an Obelus(÷), to do so call:

new JButton("\u00F7");