8
\$\begingroup\$

I created a basic desktop calculator using Java. Does anyone have any suggestions as far as coding best practices, readability issues, or just generic coding mistakes I might have missed? Any professional input you could provide would be greatly appreciated.

Calculator.java

package calc;
import javax.swing.JFrame;

public class Calculator {
    public static void main(String[] args) {
        CalculatorGUI window = new CalculatorGUI("Basic Calculator");
        window.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        window.setSize(300, 400);
        window.setFocusable(true);
        window.setVisible(true);
    }
}

CalculatorGUI.java

package calc;

import java.awt.Color;
import java.awt.Font;
import java.awt.GridBagConstraints;
import java.awt.GridBagLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.KeyEvent;
import java.awt.event.KeyListener;

import javax.swing.JFrame;
import javax.swing.JButton;
import javax.swing.JLabel;
import javax.swing.SwingConstants;
import javax.swing.border.EmptyBorder;


public class CalculatorGUI extends JFrame implements ActionListener, KeyListener {

    private static final long serialVersionUID = 1L;
    private Integer [] placeholder = new Integer[15];
    private long placeholderBuilder;
    private double numberOne = 0;
    private double numberTwo = 0;
    private double total = 0;
    private String operand;
    private boolean positiveNumberSwitch = true;

    private int testingConsoleDisplayCounter = 1;

    private long beforeDecimal;
    private long afterDecimal;
    private boolean isDecimal;

    private GridBagLayout layout = new GridBagLayout();

    private JLabel displayLabel = new JLabel("0");

    private JButton nineButton  = new JButton("9");
    private JButton eightButton = new JButton("8");
    private JButton sevenButton = new JButton("7");
    private JButton sixButton   = new JButton("6");
    private JButton fiveButton  = new JButton("5");
    private JButton fourButton  = new JButton("4");
    private JButton threeButton = new JButton("3");
    private JButton twoButton   = new JButton("2");
    private JButton oneButton   = new JButton("1");
    private JButton zeroButton  = new JButton("0");

    private JButton clearButton    = new JButton("C");
    private JButton posnegButton   = new JButton("-/+");
    private JButton deleteButton   = new JButton("<-");
    private JButton divideButton   = new JButton("/");
    private JButton timesButton    = new JButton("*");
    private JButton subtractButton = new JButton("-");
    private JButton addButton      = new JButton("+");
    private JButton decimalButton  = new JButton(".");
    private JButton equalsButton   = new JButton("=");

    public CalculatorGUI(String title){
        super(title);
        setLayout(layout);
        // makePanel (display text, Layout type, cell span, height, width, row number, column number)

        makeLabel(displayLabel, layout, GridBagConstraints.REMAINDER, 1, 1, 0, 1);

        // makeButton (display text, Layout type, cell span, height, width, row number, column number)

        makeButton(clearButton,   layout, 1, 1, 1, 1, 1);
        makeButton(posnegButton,  layout, 1, 1, 1, 1, 2);
        makeButton(deleteButton,  layout, 1, 1, 1, 1, 3);
        makeButton(divideButton,  layout, 1, 1, 1, 1, 4);

        makeButton(sevenButton, layout, 1, 1, 1, 2, 1);
        makeButton(eightButton, layout, 1, 1, 1, 2, 2);
        makeButton(nineButton,  layout, 1, 1, 1, 2, 3);
        makeButton(timesButton, layout, 1, 1, 1, 2, 4);

        makeButton(fourButton,     layout, 1, 1, 1, 3, 1);
        makeButton(fiveButton,     layout, 1, 1, 1, 3, 2);
        makeButton(sixButton,      layout, 1, 1, 1, 3, 3);
        makeButton(subtractButton, layout, 1, 1, 1, 3, 4);

        makeButton(oneButton,   layout, 1, 1, 1, 4, 1);
        makeButton(twoButton,   layout, 1, 1, 1, 4, 2);
        makeButton(threeButton, layout, 1, 1, 1, 4, 3);
        makeButton(addButton,   layout, 1, 1, 1, 4, 4);

        makeButton(zeroButton,    layout, 1, 1, 1, 5, 1);
        makeButton(decimalButton, layout, 1, 1, 1, 5, 2);
        makeButton(equalsButton,  layout, GridBagConstraints.REMAINDER, 1, 1, 5, 3);

        // Adds Keyboard key listener
        this.addKeyListener(this);

    }

    // Method used to create and format the top JLabel that displays the entered numbers and end total
    void makeLabel(JLabel label, GridBagLayout gd, int gridwidth, int weightx, int weighty, int gridy, int gridx){
        GridBagConstraints c = new GridBagConstraints();

        label.setBorder(new EmptyBorder(0, 5, 0, 25) );
        label.setFont(new Font("monospaced", Font.BOLD, 26));
        label.setHorizontalAlignment(SwingConstants.RIGHT);

        c.gridwidth = gridwidth;
        c.weightx = weightx;
        c.weighty = weighty;
        c.gridx = gridx;
        c.gridy = gridy;
        c.fill = GridBagConstraints.BOTH;
        gd.setConstraints(label, c);
        add(label);


    }

    // Method used to create and format the GUI JButtons
    void makeButton(JButton button, GridBagLayout gd, int gridwidth, int weightx, int weighty, int gridy, int gridx){
        GridBagConstraints c = new GridBagConstraints();

        button.setFont(new Font("monospaced", Font.BOLD, 21));
        button.setForeground(Color.DARK_GRAY);
        button.setBackground(Color.LIGHT_GRAY);

        c.gridwidth = gridwidth;
        c.weightx = weightx;
        c.weighty = weighty;
        c.gridx = gridx;
        c.gridy = gridy;
        c.fill = GridBagConstraints.BOTH;
        gd.setConstraints(button, c);
        button.addActionListener(this);
        add(button);
    }

    // Event Listener for the button clicks
    public void actionPerformed(ActionEvent e) {

        switch (e.getActionCommand()){
            case "9":   numberAction(9);
                        break;

            case "8":   numberAction(8);
                        break;

            case "7":   numberAction(7);
                        break;

            case "6":   numberAction(6);
                        break;

            case "5":   numberAction(5);
                        break;

            case "4":   numberAction(4);
                        break;

            case "3":   numberAction(3);
                        break;

            case "2":   numberAction(2);
                        break;

            case "1":   numberAction(1);
                        break;

            case "0":   numberAction(0);
                        break;

            case("+"):  operand = "+";
                        functionForMultipuleEntryPastTwoNumbers();
                        clear();
                        break;

            case("-"):  operand = "-";
                        functionForMultipuleEntryPastTwoNumbers();
                        clear();
                        break;

            case("*"):  operand = "*";
                        functionForMultipuleEntryPastTwoNumbers();
                        clear();
                        break;

            case("/"):  operand = "/";
                        functionForMultipuleEntryPastTwoNumbers();
                        clear();
                        break;

            case("="):  makeNumberTwo();
                        total();
                        clear();
                        placeholder[0] = 0;
                        isDecimal = false;
                        break;

            case("."):  isDecimal = true;
                        makeNumberBeforeDecimal();
                        break;

            case("C"):  for (int i = 0; i < placeholder.length; i++) {
                            placeholder[i] = null;
                        }
                        displayLabel.setText("0");
                        numberOne = 0;
                        numberTwo = 0;
                        total = 0;
                        placeholder[0] = 0;
                        isDecimal = false;
                        break;

            case("<-"): backspace();
                        numberAction();
                        break;

            case("-/+"): 
                        if (positiveNumberSwitch == true){
                            positiveNumberSwitch = false;
                        }else{
                            positiveNumberSwitch = true;;
                        }
                        if(placeholder[0] != null){
                            numberAction();
                        }
                        break;
        }
    }

    // Event Listener to allow the user to enter numbers and operands via their keyboard.
    public void keyTyped(KeyEvent e) {

        switch (e.getKeyChar()){
            case '9':   numberAction(9);
                        break;

            case '8':   numberAction(8);
                        break;

            case '7':   numberAction(7);
                        break;

            case '6':   numberAction(6);
                        break;

            case '5':   numberAction(5);
                        break;

            case '4':   numberAction(4);
                        break;

            case '3':   numberAction(3);
                        break;

            case '2':   numberAction(2);
                        break;

            case '1':   numberAction(1);
                        break;

            case '0':   numberAction(0);
                        break;

            case('+'):  operand = "+";
                        functionForMultipuleEntryPastTwoNumbers();
                        clear();
                        break;

            case('-'):  operand = "-";
                        functionForMultipuleEntryPastTwoNumbers();
                        clear();
                        break;

            case('*'):  operand = "*";
                        functionForMultipuleEntryPastTwoNumbers();
                        clear();
                        break;

            case('/'):  operand = "/";
                        functionForMultipuleEntryPastTwoNumbers();
                        clear();
                        break;

            case('='):  makeNumberTwo();
                        total();
                        clear();
                        isDecimal = false;
                        break;

            case('.'):  isDecimal = true;
                        makeNumberBeforeDecimal();
                        break;

            case('C'):  clear();
                        displayLabel.setText("0");
                        total = 0;
                        placeholder[0] = 0;
                        isDecimal = false;
                        break;
        }

        switch((int)e.getKeyChar()){
                        // Backspace Button
            case(8):    backspace();
                        numberAction();
                        break;

                        // Enter Button
            case(10):   makeNumberTwo();
                        total();
                        clear();
                        isDecimal = false;
                        placeholder[0] = 0;
                        break;

        }
    }

    @Override
    public void keyPressed(KeyEvent e) {

    }

    @Override
    public void keyReleased(KeyEvent e) {

    }

    // Method used to display button value on the JLabel and add value to array.
    private void numberAction(){
        makePlaceholderNumber();
        displayLabel.setText(Long.toString(placeholderBuilder));
    }   

    // Method used to take a value of a button or key number entered by the user and add it to the placeholder array.
    private void numberAction(int buttonNumber){
        for (int i = 0; i < placeholder.length; i++) {
            if (placeholder[i] == null) {
                placeholder[i]= buttonNumber;
                break;
            }
        }
        makePlaceholderNumber();
        if(isDecimal == true){
            displayLabel.setText(beforeDecimal + "." +Long.toString(placeholderBuilder));
        }else{
            displayLabel.setText(Long.toString(placeholderBuilder));
        }
    }

    // Method converts to numbers in the placeholder [] array into a placeholderBuilder string to be displayed in the display JLabel later.
    private void makePlaceholderNumber(){
        StringBuilder number = new StringBuilder();
        for (Integer num : placeholder){
            if (num != null){
                number.append(num);
            }
        }

        if (positiveNumberSwitch == false){
            long displayNumber = Long.parseLong(number.toString());
            displayNumber = -displayNumber;
            placeholderBuilder = displayNumber;
        }else{
            placeholderBuilder = Long.parseLong(number.toString());
        }

        testingStringOutputToConsole();
    }

    private void makeNumberOne(){
        StringBuilder number = new StringBuilder();
        for (Integer num : placeholder){
            if (num != null){
                number.append(num);
            }
        }
        if (isDecimal == true){
            makeNumberAfterDecimal();
            String x = (Long)beforeDecimal + "." + (Long)afterDecimal;

            numberOne = Double.parseDouble(x);
        }else{
            numberOne = Double.parseDouble(number.toString());
        }

        if (positiveNumberSwitch == false){
                numberOne = -numberOne;
            }

        testingStringOutputToConsole();
    }

    private void makeNumberTwo(){
        StringBuilder number = new StringBuilder();
        for (Integer num : placeholder){
            if (num != null){
                number.append(num);
            }
        }

        if (isDecimal == true){
            makeNumberAfterDecimal();
            String tempString = (Long)beforeDecimal + "." + (Long)afterDecimal;
            numberTwo = Double.parseDouble(tempString);
        }else{
            numberTwo = Double.parseDouble(number.toString());
        }


        if (positiveNumberSwitch == false){
            numberTwo = -numberTwo;
        }

        testingStringOutputToConsole();

    }

    // Method used to clear the enter the placeholder array and number total as well as reset positive/negative boolean indicator.
    private void clear(){
        for (int i = 0; i < placeholder.length; i++) {
              placeholder[i] = null;
        }
        positiveNumberSwitch = true;
        total = 0;

        testingStringOutputToConsole();

    }

    // Method used to delete the last number entered by the user.
    private void backspace(){
        placeholder[placeholder.length - 2] = null;

        for (int i = 0; i < placeholder.length; i++) {
            if (placeholder[i] == null) {
                placeholder[i - 1] = null;
                break;
            }
        }

        testingStringOutputToConsole();

    }

    //Method generates the total of the two number and displays it of the displayLabel.
    private void total(){
        switch(operand){
            case("+"):  total = numberOne + numberTwo;
                        displayLabel.setText((total % 1 == 0) ? Long.toString((long)total) : Double.toString(total));
                        break;


            case("-"):  total = numberOne - numberTwo;
                        displayLabel.setText((total % 1 == 0) ? Long.toString((long)total) : Double.toString(total));
                        break;

            case("*"):  total = numberOne * numberTwo;
                        displayLabel.setText((total % 1 == 0) ? Long.toString((long)total) : Double.toString(total));
                        break;


            case("/"):  total = numberOne / numberTwo;
                        displayLabel.setText((total % 1 == 0) ? Long.toString((long)total) : Double.toString(total));
                        break;
        }
    }

    //Method used to allow for the entering of more than 2 numbers before a equals " = ", for example ( 12 + 3256 + 9752 + ..... )
    private void functionForMultipuleEntryPastTwoNumbers(){
        if(numberOne == 0){
            makeNumberOne();
        }else{
            makeNumberTwo();
            total();
            numberOne = total;
            numberTwo = 0;
        }
        isDecimal = false;
    }

    //Method used to generate the numbers BEFORE a user entered decimal " . " point
    private void makeNumberBeforeDecimal(){
        StringBuilder number = new StringBuilder();
        for (Integer num : placeholder){
            if (num != null){
                number.append(num);
            }
        }
        beforeDecimal = Integer.parseInt(number.toString());

        if (positiveNumberSwitch == false){
            beforeDecimal = -beforeDecimal;
        }

        clear();
    }

    //Method used to generate the numbers AFTER a user entered decimal " . " point
    private void makeNumberAfterDecimal(){
        StringBuilder number = new StringBuilder();
        for (Integer num : placeholder){
            if (num != null){
                number.append(num);
            }
        }
        afterDecimal = Integer.parseInt(number.toString());
        clear();
    }

    //Method used to print out text on the console that can assist in seeing what is actually happening
    private void testingStringOutputToConsole(){
        System.out.println("-------------------------");
        System.out.println(testingConsoleDisplayCounter++);
        for (int j = 0; j < placeholder.length; j++) {
            System.out.println("placeholder[" +j+ "] =" + placeholder[j] + ";");
        }
        System.out.println("numberOne: " + numberOne);
        System.out.println("numberTwo: " + numberTwo);
        System.out.println("Total: "     + total);
    }

}
\$\endgroup\$
  • \$\begingroup\$ idea is good, code and answers are wrong (excluding invokeLater), use JFormattedTextField (every problems), actioncommand can be used for KeyBindings instead of KeyListener (label from JButton == accelerator for KeyBindings), I'd be use JFormattedTextField with JTextArea (for history, both without borders creates compact object) \$\endgroup\$ – mKorbel Aug 1 '14 at 13:00
  • \$\begingroup\$ placeholder is about input numbers, hmmm similair as is for ATM (wihout using of decimal separator) or for negative sing \$\endgroup\$ – mKorbel Aug 1 '14 at 13:02
6
\$\begingroup\$

This isn't a full review (yet), but some hopefully helpful comments.

  • You should always start your Swing application by calling SwingUtilities.invokeLater. This Oracle/Sun tutorial explains why very well.

  • Your calculator would be able to support longer equations in a much simpler fashion if you instead placed your symbols in a stack. Then you'd be able to support an arbitrary number of statements, plus parentheticals, and would find your total by just popping each successive element from the stack and applying the appropriate operation (if any). This would make the functionForMultipuleEntryPastTwoNumbers() obsolete since it would support any number of numbers and operands.

  • Using a double for the numberOne, numberTwo, and total variables is inviting a loss of precision. Consider float BigDecimal instead.

  • You don't have any checks against invalid operations. What if a user enters 2 / 0? Or if they enter a number that's too long to store in a double or the placeholder array?

  • Instead of the testingStringOutputToConsole() method, you may want to consider writing unit tests using a framework like JUnit to verify your functionality.

  • I'm not understanding the purpose of the placeholder array. Maybe, instead have a String for the entered value? Then when the user enters a symbol, use Double.parseDouble or Integer.parseInt as appropriate to convert the previously entered value to the appropriate numeric type. Right now, if you enter 1.02, your method to convert that to a number would result in 1.2 because it converts the digits prior to the decimal to an int (1) and then the digits after the decimal to an int (2), builds a new string (1.2), and parses the decimal.

\$\endgroup\$
  • \$\begingroup\$ Loss of precision? double has better precision than float! \$\endgroup\$ – Simon Forsberg Jul 11 '14 at 16:10
  • \$\begingroup\$ I actually meant BigDecimal but clearly my mind is not all here this morning... @SimonAndréForsberg \$\endgroup\$ – Roddy of the Frozen Peas Jul 11 '14 at 16:17
6
\$\begingroup\$

Unnecessary class variables

private JButton nineButton  = new JButton("9");
private JButton eightButton = new JButton("8");
private JButton sevenButton = new JButton("7");

You don't need to define these as variables, you can create them directly in your constructor:

makeButton(new JButton("7"), layout, 1, 1, 1, 2, 1);
makeButton(new JButton("8"), layout, 1, 1, 1, 2, 2);
...

JavaDoc

You comment some methods by adding a // comment before the method, a better alternative would be JavaDoc. Entering /** and pressing enter in your IDE will probably create a JavaDoc block.

Here's how JavaDoc looks like:

/**
 * Method used to clear the enter the placeholder array and number total as well as reset positive/negative boolean indicator.
 */
private void clear(){

Swing comments

Instead of using Swing, consider learning JavaFX. JavaFX makes a better separation of your code and the design of your form.

Technically, it is not recommended to extend JFrame in Swing, it is preferred to use composition over inheritance. This is unfortunately something that is quite common in Swing. In JavaFX though, it is much easier to avoid that.


Boolean not operator

This code:

if (positiveNumberSwitch == true){
    positiveNumberSwitch = false;
}else{
    positiveNumberSwitch = true;;
}

Can be written like this:

positiveNumberSwitch = !positiveNumberSwitch;

! is the boolean not operator.


switch

I'm not a big fan of how you have structured your switch and case statements. Especially with regards to the indentation.

You can use two tricks here to reduce the code significantly.

  1. Character.isDigit and Character.digit methods for digits.
  2. Group multiple cases together and use the e.getKeyChar() for the operand variable.

Here's how I would write the first part of that method:

    if (Character.isDigit(e.getKeyChar())) {
        numberAction(Character.digit(e.getKeyChar(), 10));
    }
    switch (e.getKeyChar()){
        case '+':
        case '-':
        case '*':
        case '/':
            operand = String.valueOf(e.getKeyChar());
            functionForMultipuleEntryPastTwoNumbers();
            clear();
            break;

        case '=':
            makeNumberTwo();
            total();
            clear();
            isDecimal = false;
            break;

        case '.':
            isDecimal = true;
            makeNumberBeforeDecimal();
            break;

        case 'c': // Note this 'c' as otherwise to add case-insensitivity
        case 'C':
            clear();
            displayLabel.setText("0");
            total = 0;
            placeholder[0] = 0;
            isDecimal = false;
            break;
    }

I can say positive things too

I like that you have separated your main method from your GUI class.


If statements with boolean conditions

if (isDecimal == true){
if (positiveNumberSwitch == false){

Using == true is redundant and == false can be written with a ! at the beginning instead. (Again, the boolean not operator)

Also, I would add a space between ) and {

if (isDecimal) {
if (!positiveNumberSwitch) {

Speaking of spaces, }else{ would look better as } else {


total method

In your total method, you write this inside each case:

displayLabel.setText((total % 1 == 0) ? Long.toString((long)total) : Double.toString(total));

Instead do that after the switch:

Here's a clean-up version of your total method:

/**
 * Method generates the total of the two number and displays it of the displayLabel.
 */
private void total() {
    switch (operand) {
        case "+":
            total = numberOne + numberTwo;
            break;
        case "-":
            total = numberOne - numberTwo;
            break;
        case "*":
            total = numberOne * numberTwo;
            break;
        case "/":
            total = numberOne / numberTwo;
            break;
    }
    displayLabel.setText((total % 1 == 0) ? Long.toString((long)total) : Double.toString(total));
}

Pay extra attention to:

  • The difference in indentation.
  • JavaDoc at beginning of method
  • No parenthesis at each case label.

There is plenty of things to be said about your code, this was only some of them. Perhaps others will add more.

\$\endgroup\$
  • \$\begingroup\$ Thanks Simon, you noted some good points that I over looked. They seem like obvious mistakes now that someome else has pointed them out. Thanks again for your review. \$\endgroup\$ – Broomes Jul 11 '14 at 18:38

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.