A (late) Simple Calculator

Question

As I was looking through my past questions, I noticed my really old calculator question (here and here). Considering it looked like a huge mess, I decided to rewrite it. Coincidentally, the April 2015 Community Challenge was to implement a simple calculator. It might be a bit late, but here it is:

import java.util.HashMap;
import java.util.Map;
import java.util.regex.Pattern;

public class Calculator {

    private static final Pattern NUMBER_PATTERN = Pattern
            .compile("\\d+(\\.\\d+)?");

    public static double calculate(String equation) {
        return calculate(equation, Operation.getLastInPrecedence());
    }

    private static double calculate(String equation, Operation currentOp) {
        Operation nextOp = currentOp.getPreviousInPrecedence();
        if (NUMBER_PATTERN.matcher(equation).matches()) {
            return Double.parseDouble(equation);
        }
        String[] equationParts = equation.split("\\s*"
                + Pattern.quote(Character.toString(currentOp.getSymbol()))
                + "\\s*");
        double result;
        if (nextOp == null) {
            // Last operation
            result = Math.pow(Integer
                    .parseInt(equationParts[equationParts.length - 2]), Integer
                    .parseInt(equationParts[equationParts.length - 1]));
            for (int i = equationParts.length - 3; i >= 0; i--) {
                result = Math.pow(Integer.parseInt(equationParts[i]), result);
            }
        } else {
            result = calculate(equationParts[0], nextOp);
            for (int i = 1; i < equationParts.length; i++) {
                result = currentOp.execute(result, calculate(equationParts[1],
                        nextOp));
            }
        }
        return result;
    }

}

class Operation {

    public static final Operation ADD = new Operation('+', 1);
    public static final Operation SUBTRACT = new Operation('-', 2);
    public static final Operation MULTIPLY = new Operation('*', 3);
    public static final Operation DIVIDE = new Operation('/', 4);

    public static final Operation POW = new Operation('^', 5);

    private static final Map<Integer, Operation> operations = new HashMap<>();

    static {
        operations.put(ADD.precedence, ADD);
        operations.put(SUBTRACT.precedence, SUBTRACT);
        operations.put(MULTIPLY.precedence, MULTIPLY);
        operations.put(DIVIDE.precedence, DIVIDE);
        operations.put(POW.precedence, POW);
    }

    private final char symbol;
    private final int precedence;

    private Operation(char symbol, int precedence) {
        this.symbol = symbol;
        this.precedence = precedence;
    }

    public char getSymbol() {
        return symbol;
    }

    public int getPrecedence() {
        return precedence;
    }

    public double execute(double left, double right) {
        if (this == ADD) {
            return left + right;
        } else if (this == SUBTRACT) {
            return left - right;
        } else if (this == MULTIPLY) {
            return left * right;
        } else if (this == DIVIDE) {
            return left / right;
        } else if (this == POW) {
            return Math.pow(left, right);
        } else {
            // Not Possible
            throw new InternalError();
        }
    }

    public Operation getNextInPrecedence() {
        return operations.get(precedence - 1);
    }

    public Operation getPreviousInPrecedence() {
        return operations.get(precedence + 1);
    }

    public static Operation getFromPrecedence(int precedence) {
        return operations.get(precedence);
    }

    public static Operation getLastInPrecedence() {
        return ADD;
    }

    public static Operation getFirstInPrecedence() {
        return POW;
    }

}

The logic here is fairly simple:

• I have a recursive solution that goes through the operations, in reverse precedence. This way, the highest precedence operations are calculated first.
• The method will first check if the given equation is a number. If it is, it just returns the parsed number.
• If not, it continues executing. It will first split the equation into the parts that are separated by the current operation, and calculates them by recursively calling this method.
• If it is the last operation (i.e. pow or ^), then it will perform a different method, going from right to left. Explanation will appear later in this post. Explanation:

In addition (or subtraction or multiplication or whatever), you go from left to right:

1 + 2 + 3

is essentially the same as:

$$1 + 2 + 3$$

but with pow, you have to go from right to left.

2 ^ 3 ^ 2

Is the same as:

$$2 ^ {3 ^ 2}$$

As long as you go from right to left. If you go from left to right though, it will result in:

$$(2 ^ 3) ^ 2$$

\$2 ^ {3 ^ 2}\$ is 512, which does not equal \$(2 ^ 3) ^ 2\$ (64).

• Once all the operations are complete, it will either return the result, or throw an exception due to a over-complicated equation.

I also have tests:

class Test {

    @Test
    public void testAdd() {
        assertEquals(6.0, Calculator.calculate("3.5 + 2.5"));
    }

    @Test
    public void testSubtract() {
        assertEquals(1.0, Calculator.calculate("3.5 - 2.5"));
    }

    @Test
    public void testMultiply() {
        assertEquals(8.75, Calculator.calculate("3.5 * 2.5"));
    }

    @Test
    public void testDivide() {
        assertEquals(1.4, Calculator.calculate("3.5 / 2.5"));
    }

    @Test
    public void testPow() {
        assertEquals(9.0, Calculator.calculate("3 ^ 2"));
    }

}

Major concerns:

1. The code seems to be fixed for this specific problem. If I were to add another operation, the design has to be changed a lot. Is there a way to design it in a more flexible way?
2. Are my tests fine?
3. Does it do the job in the most efficient way?

Answer 1

I have doubts about your unit tests — did you run them?

1. The test runner can't run your test unless it is public.
2. The class name Test collides with the org.junit.Test annotation.
3. For all the tests, I got java.lang.AssertionError: Use assertEquals(expected, actual, delta) to compare floating-point numbers — you need to specify a tolerance.

In addition, your test coverage is spotty.

4. You mentioned your concern about right-to-left associativity for the ^ operator, so why didn't you test for that?
5. Is 1 + 1 / 2 equal to 1.5 (standard precedence rules) or 1.0 (cheap calculator)?
6. Is 12 / 2 / 3 equal to 2.0 (standard associativity rules)? Your code evaluates it to 3.0, failing this test.

As for the calculator itself, I think it would be worthwhile to implement support for parentheses as well. Otherwise, there's no way to override the order of operations.

Answer 2

The nice thing about Enums is that they can be used in switch statements. Your code here:

public double execute(double left, double right) {
    if (this == ADD) {
        return left + right;
    } else if (this == SUBTRACT) {
        return left - right;
    } else if (this == MULTIPLY) {
        return left * right;
    } else if (this == DIVIDE) {
        return left / right;
    } else if (this == POW) {
        return Math.pow(left, right);
    } else {
        // Not Possible
        throw new InternalError();
    }
}

Could easily be:

public double execute(double left, double right) {
    switch (this) {
        case ADD:
            return left + right;
        case SUBTRACT:
            return left - right;
        case MULTIPLY:
            return left * right;
        case DIVIDE:
            return left / right;
        case POW:
            return Math.pow(left, right);
    }
    throw new IllegalStateException("Unchecked case " + this);
}

You could do that if you converted Operation to be an enum (which I recommend you do).

Note that I have changed the Error to an Exception too (with a decent message).

I would be really tempted to suggest a Java8 function though, instead. Something where you have a constructor for the Operator....

If Operator was an enum, and took a BinaryDoubleOperator, then it could be simple...

public enum Operator{
    ADD('+', 1, (a,b) -> a + b),
    SUBTRACT('-', 2, (a,b) -> a - b),
    ....;

    private final DoubleBinaryOperator calculation;
    private final char symbol;
    private final int precedence;

    Operator(char symbol, int precedence, DoubleBinaryOperator calc) {
        this.symbol = symbol;
        this.precedence = precedence;
        this.calculation = calc;
    }

    public double execute(double left, double right) {
        return calculation.applyAsDouble(left, right);
    }

    ....
}

Apart from that, I like the recursion, and so on. You have avoided one common bug with precedence by specifying / as being higher than *. I am not sure if that's fair though... most languages treat them the same.