Writing a calculator in idiomatic Java

Question

I just dipped my toes in Java today (coming from C++) and made a few things to learn it. The standard stuff like Atari Breakout, Tic Tac Toe and this calculator. This has been done so many times before!

Calculator.java

import java.math.*;

public class Calculator {
  private static final int MAX_DIGITS = 10;
  private static final MathContext ARITH_ROUND = new MathContext(
    MAX_DIGITS * 2, RoundingMode.HALF_EVEN
  );
  private static final MathContext SCREEN_ROUND = new MathContext(
    MAX_DIGITS, RoundingMode.HALF_EVEN
  );

  public enum Operation {
    NONE, ADD, SUB, MUL, DIV, EQUALS
  }

  private Operation prevOp;
  private BigDecimal prevNumber;
  private String currNumberStr;
  private boolean error = false;

  public Calculator() {
    prevOp = Operation.NONE;
    prevNumber = null;
    currNumberStr = new String();
  }

  public void appendDigit(char digit) {
    if (currNumberStr.length() >= MAX_DIGITS) {
      return;
    }
    if (digit == '.') {
      if (currNumberStr.indexOf('.') != -1) {
        return;
      }
      if (currNumberStr.isEmpty()) {
        currNumberStr += '0';
      }
    } else if (digit == '0') {
      if (currNumberStr.isEmpty() || currNumberStr.equals("0")) {
        return;
      }
    }
    currNumberStr += digit;
  }

  private BigDecimal applyOp(Operation op, BigDecimal a, BigDecimal b) {
    switch (op) {
      case NONE:
        assert false;
        break;
      case ADD:
        return a.add(b, ARITH_ROUND);
      case SUB:
        return a.subtract(b, ARITH_ROUND);
      case MUL:
        return a.multiply(b, ARITH_ROUND);
      case DIV:
        return a.divide(b, ARITH_ROUND);
      case EQUALS:
        return a;
    }
    return null;
  }

  private BigDecimal parseCurrNumber() {
    if (currNumberStr.isEmpty()) {
      return new BigDecimal("0");
    } else {
      return new BigDecimal(currNumberStr);
    }
  }

  public void applyOp(Operation op) {
    BigDecimal currNumber = null;
    if (prevOp == Operation.NONE) {
      if (prevNumber == null) {
        currNumber = parseCurrNumber();
      } else {
        currNumber = prevNumber;
      }
    } else if (error) {
      currNumber = parseCurrNumber();
      error = false;
    } else {
      try {
        currNumber = applyOp(prevOp, prevNumber, parseCurrNumber());
        error = false;
      } catch (ArithmeticException e) {
        currNumber = null;
        error = true;
        op = Operation.NONE;
      }
    }
    prevNumber = currNumber;
    currNumberStr = "";
    prevOp = op;
  }

  public void clear() {
    error = false;
    if (currNumberStr.isEmpty()) {
      prevNumber = null;
      prevOp = Operation.NONE;
    } else {
      currNumberStr = "";
    }
  }

  private static String numToString(BigDecimal num) {
    return num.round(SCREEN_ROUND).toString();
  }

  private interface UnaryOp {
    public BigDecimal apply(BigDecimal num);
  }

  private void applyUnaryOp(UnaryOp op) {
    if (currNumberStr.isEmpty()) {
      if (prevNumber != null) {
        prevNumber = op.apply(prevNumber);
        if (prevNumber == null) {
          error = true;
        }
      }
    } else {
      BigDecimal currNumber = new BigDecimal(currNumberStr);
      currNumber = op.apply(currNumber);
      if (currNumber == null) {
        error = true;
        currNumberStr = "";
      } else {
        currNumberStr = numToString(currNumber);
      }
    }
  }

  public void negate() {
    applyUnaryOp((BigDecimal n) -> n.negate(ARITH_ROUND));
  }

  public void root() {
    applyUnaryOp((BigDecimal n) -> {
      double val = n.doubleValue();
      if (val < 0.0) {
        return null;
      } else {
        return new BigDecimal(Math.sqrt(val));
      }
    });
  }

  public String screen() {
    if (currNumberStr.isEmpty()) {
      if (error) {
        return "Error";
      } else  if (prevNumber == null) {
        return "0";
      } else {
        return numToString(prevNumber);
      }
    } else {
      return currNumberStr;
    }
  }
}

Frame.java

import java.awt.*;
import java.awt.event.*;
import javax.swing.*;

class ButtonColorListener extends MouseAdapter {
  public static final Color DOWN_COLOR = Color.GRAY;
  public static final Color UP_COLOR = Color.DARK_GRAY;

  private JButton button;

  public ButtonColorListener(JButton newButton) {
    button = newButton;
  }

  @Override public void mousePressed(MouseEvent e) {
    button.setBackground(DOWN_COLOR);
  }

  @Override public void mouseReleased(MouseEvent e) {
    button.setBackground(UP_COLOR);
  }
}

public class Frame extends JFrame {
  private Calculator calc;
  private JLabel screen;

  private void updateScreen() {
    screen.setText(calc.screen());
  }

  private static final Font BUTTON_FONT = new Font(Font.MONOSPACED, Font.PLAIN, 40);
  private static final Font SCREEN_FONT = new Font(Font.MONOSPACED, Font.PLAIN, 32);

  // Is there a shorthand for this?
  // (so that I don't have to declare an interface?)
  private interface CalcAction {
    public void apply();
  }

  private void addButton(int x, int y, int w, int h, String name, CalcAction action) {
    GridBagConstraints c = new GridBagConstraints();
    c.gridx = x;
    c.gridy = y;
    c.gridwidth = w;
    c.gridheight = h;
    //                    top, left, bottom, right
    c.insets = new Insets(1, x != 0 ? 1 : 0, 0, 0);
    c.fill = GridBagConstraints.BOTH;

    JButton button = new JButton(name);
    button.setFocusable(false);
    button.setPreferredSize(new Dimension(64 * w, 64 * h));
    button.setBorder(null);
    button.setBackground(ButtonColorListener.UP_COLOR);
    button.setForeground(Color.WHITE);
    button.setOpaque(true);
    button.setFont(BUTTON_FONT);
    button.addActionListener(new ActionListener() {
      @Override public void actionPerformed(ActionEvent e) {
        action.apply();
        updateScreen();
      }
    });
    button.addMouseListener(new ButtonColorListener(button));

    getContentPane().add(button, c);
  }

  private void addButton(int x, int y, String name, CalcAction action) {
    addButton(x, y, 1, 1, name, action);
  }

  private void addScreen() {
    GridBagConstraints c = new GridBagConstraints(); 
    c.gridx = 0;
    c.gridy = 0;
    c.gridwidth = 4;
    c.anchor = GridBagConstraints.EAST;

    screen = new JLabel();
    screen.setBackground(Color.BLACK);
    screen.setForeground(Color.WHITE);
    screen.setOpaque(true);
    screen.setFont(SCREEN_FONT);

    getContentPane().add(screen, c);
  }

  public Frame() {
    super("Calculator");

    calc = new Calculator();

    setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    getContentPane().setLayout(new GridBagLayout());
    getContentPane().setBackground(Color.BLACK);

    addScreen();    
    updateScreen();

    addButton(0, 1, "C", () -> calc.clear());
    // Do I need to treat non-ascii characters differently?
    // It seems to work as-is
    addButton(1, 1, "±", () -> calc.negate());
    addButton(2, 1, "√", () -> calc.root());
    addButton(3, 1, "÷", () -> calc.applyOp(Calculator.Operation.DIV));
    addButton(0, 2, "7", () -> calc.appendDigit('7'));
    addButton(1, 2, "8", () -> calc.appendDigit('8'));
    addButton(2, 2, "9", () -> calc.appendDigit('9'));
    addButton(3, 2, "×", () -> calc.applyOp(Calculator.Operation.MUL));
    addButton(0, 3, "4", () -> calc.appendDigit('4'));
    addButton(1, 3, "5", () -> calc.appendDigit('5'));
    addButton(2, 3, "6", () -> calc.appendDigit('6'));
    addButton(3, 3, "-", () -> calc.applyOp(Calculator.Operation.SUB));
    addButton(0, 4, "1", () -> calc.appendDigit('1'));
    addButton(1, 4, "2", () -> calc.appendDigit('2'));
    addButton(2, 4, "3", () -> calc.appendDigit('3'));
    addButton(3, 4, "+", () -> calc.applyOp(Calculator.Operation.ADD));
    addButton(0, 5, 2, 1, "0", () -> calc.appendDigit('0'));
    addButton(2, 5, ".", () -> calc.appendDigit('.'));
    addButton(3, 5, "=", () -> calc.applyOp(Calculator.Operation.EQUALS));

    pack();
    setLocationRelativeTo(null);
    setVisible(true);
  }

  public static void main(String[] args) {
    // this feels strange
    new Frame();
  }
}

The main reason the implementation of Calculator is so messy is that I tried my very best to replicate the behaviour of the calculator the comes with macOS. I am aware of two minor details where the two differ but life is too short to worry about these things.

Calculator.prevNumber may or may not be null. Calculator.currNumberStr may or may not be empty. Calculator.error may or may not be true. I'm not looking for a review that tells me that I should split these 8 states (minus the invalid ones) into an enum or separate classes. My main concern is style, naming conventions and the way that I use the language and standard library. I want to know if I'm using a feature strangely or if I should be using a different feature. I'm looking for Java specific feedback, not language agnostic "use a better algorithm" feedback.

I want to write Java like a Java programmer.

Answer 1

You've a nice little calculator. The other answers have made excellent points. Here are some more to make it even better. ;-)

---

Calculator

Constructor

You are initializing 3 members in your constructor, and a fourth member error using a declaration initialization. You could do all 4 initializations at the declaration, in which case your constructor would be empty ... and could be omitted entirely.

private Operation prevOp = Operation.NONE;
private BigDecimal prevNumber = null;
private String currNumberStr = "";     // or new String()
private boolean error = false;

currNumberStr

You are using String concatenation to build up a string of characters representing a number. It most cases, String concatenation should be avoided in favour of using a StringBuilder, which is more efficient. Given there will be hundreds of milliseconds between adding characters to the currNumberStr, this is probably a pointless optimization. But it might be worth the practice.

final StringBuilder currNumberStr = new StringBuilder(MAX_DIGITS);

Notice that we can reuse the StringBuilder over and over, so it can be made final. Use currNumberStr.append(digit) to add characters to the buffer, currNumberStr.setLength(0) to clear it, and currNumberStr.toString() to convert it into a String for parsing or display.

interface UnaryOp

You can remove this interface, and replace uses of it with UnaryOperator<BigDecimal>.

---

Frame

ButtonColorListener

You create 19 ButtonColorListener objects, which is about 18 too many. They all do the same thing; change the button's background colour. The only difference is which button they change the background colour of...

MouseEvent.getSource() will return the Object the mouse event occurs on ... which will be the button that was pressed/released.

class ButtonColorListener extends MouseAdapter {
  public static final Color DOWN_COLOR = Color.GRAY;
  public static final Color UP_COLOR = Color.DARK_GRAY;

  @Override public void mousePressed(MouseEvent e) {
    JButton button = (JButton) e.getSource();
    button.setBackground(DOWN_COLOR);
  }

  @Override public void mouseReleased(MouseEvent e) {
    JButton button = (JButton) e.getSource();
    button.setBackground(UP_COLOR);
  }
}

With this change, you only need to create one ButtonColorListener, and add it to all of the buttons, instead of creating one per button.

private final static ButtonColorListener btnClrListener = new ButtonColorListener();

private void addButton(int x, int y, int w, int h, String name, CalcAction action) {
    // ...
    button.addMouseListener(btnClrListener);
    // ...
}

addButton

This method gives you way too much flexibility. You are creating buttons, and adding them to the contentPane() in order, left to right, top to bottom. Let the computer do the counting for you!

private final static int MAX_WIDTH = 4;
private int row = 1, column = 0;

private void addButton(String name, CalcAction action) {
    addButton(name, action, 1);
}    

private void addButton(String name, CalcAction action, int width) {
   // ...
   c.gridx = column;
   c.gridy = row;
   c.gridwidth = width;

   column += width;
   if (column >= MAX_WIDTH) {
      column = 0;
      row++;
   }
   // ...
}

Creating the Button Grid

With the above addButton method keeping track of the locations, you just need to add the buttons in the correct order (which you were doing anyway).

addButton("C", calc::clear);   // We can use method references for these first few!
addButton("±", calc::negate);
addButton("√", calc::root);
addButton("÷", () -> calc.applyOp(Calculator.Operation.DIV));
addButton("7", () -> calc.appendDigit('7'));
// ...

Since some of the methods take no arguments, we can simply use a method reference. And as mentioned in another answer, CalcAction can be replaced by Runnable.

main

Swing Application must do UI interactions on Swing's Event Dispatching Thread (EDT). Creating the UI can be an exception, since the UI is not shown until frame.setVisible(true); is called, but this is risky behaviour. It is better to simply switch to the EDT to create and display the UI.

public static void main(String[] args) {
    SwingUtilities.invokeLater(Frame::new);
}

That may feel even stranger, but it is better.

Answer 2

The Switch

private BigDecimal applyOp(Operation op, BigDecimal a, BigDecimal b) {
    switch (op) {
      case NONE:
        assert false;
        break;
      case ADD:
        return a.add(b, ARITH_ROUND);
      case SUB:
        return a.subtract(b, ARITH_ROUND);
      case MUL:
        return a.multiply(b, ARITH_ROUND);
      case DIV:
        return a.divide(b, ARITH_ROUND);
      case EQUALS:
        return a;
    }
    return null;
}

The Default Section

The statement return null can be replaced by a default-section. From oracles switch tutorial

The default section handles all values that are not explicitly handled by one of the case sections

private BigDecimal applyOp(Operation op, BigDecimal a, BigDecimal b) {
    switch (op) {
        // ..
        case EQUALS:
            return a;
        default: 
            return null;
    }
}

Polymorphism

Currently the switch tries to express: "Let's have a look, what the concrete type of op is and let me execute the correct logic for it"

This can be rewritten to: "Let op execute the correct logic."

private BigDecimal applyOp(Operation op, BigDecimal a, BigDecimal b) {
    return op.calculate(a, b);
}

For that we need to change the enum Operation. But before we change it, you need to know from Oracles Enum Tutorial

Java programming language enum types are much more powerful than their counterparts in other languages. The enum declaration defines a class (called an enum type). The enum class body can include methods and other fields. The compiler automatically adds some special methods when it creates an enum. [...]

public enum Operation {
  NONE((a, b) -> null),
  ADD((a, b) -> a.add(b, ARITH_ROUND)), 
  // ...
  EQUALS((a, b) -> a);

  BiFunction<BigDecimal, BigDecimal, BigDecimal> calculation;

  Operation(BiFunction<BigDecimal, BigDecimal, BigDecimal> calculation) {
    this.calculation = calculation;
  }

  BigDecimal calculate(BigDecimal a, BigDecimal b) {
    return calculation.apply(a, b);
  }
}

Answer 3

Starting with your questions.

// Is there a shorthand for this?
// (so that I don't have to declare an interface?)
private interface CalcAction {
    public void apply();
}

If you don't want to declare such interface you can yust use one from the standard library such as Runnable. The only difference would be that its method is called run instead of apply. Also when defining an interface all its methods are by definition public abstract. You don't have to type that explicitly.

// Do I need to treat non-ascii characters differently?
// It seems to work as-is

No need as long as you are using proper encoding such as UTF-8.

public static void main(String[] args) {
    // this feels strange
    new Frame();
}

Correct. There are two problems with this code.

• Class should not be responsible for construction of its dependencies. It should just declare what it needs to function and let the caller deal with it. This is just application of Dependency Inversion Principle (One of SOLID Principles). I.E. Having configurable dependencies.
• Constructor should not do any work except initialization of the new object. Having code in the constructor prevents you from properly using composition and would make point above very painful.

With these two points applied the main method might look something like this.

public static void main(String[] args) {
    new Frame(
        new JLabel(),
        new Calculator()
    ).start();
}

Minor remarks

When you need empty string just use literal "" no need for new String().

Instead of simple anonymous classes such as.

button.addActionListener(new ActionListener() {
    @Override
    public void actionPerformed(ActionEvent e) {
        action.apply();
        updateScreen();
    }
});

Just use lambda.

button.addActionListener(e -> {
    action.apply();
    updateScreen();
});

Assertions are turned off by default and they have to be anabled explicitly. When you have a state that is wrong, invalid or for whatever reason exceptional, throw exception.

private BigDecimal applyOp(Operation op, BigDecimal a, BigDecimal b) {
    switch (op) {
        case NONE:
            throw new IllegalStateException("NONE cannot be applied!");
        case ADD:
            return a.add(b, ARITH_ROUND);
        case SUB:
            return a.subtract(b, ARITH_ROUND);
        case MUL:
            return a.multiply(b, ARITH_ROUND);
        case DIV:
            return a.divide(b, ARITH_ROUND);
        case EQUALS:
            return a;
    }
    return null;
}