15
\$\begingroup\$

Inspired by some older questions, I decided to create my own postfix calculator using Java 8. I'd like to have all aspects reviewed.

public enum Operator implements DoubleBinaryOperator {
    PLUS    ("+", (l, r) -> l + r),
    MINUS   ("-", (l, r) -> l - r),
    MULTIPLY("*", (l, r) -> l * r),
    DIVIDE  ("/", (l, r) -> l / r);

    private final String symbol;
    private final DoubleBinaryOperator binaryOperator;

    private Operator(final String symbol, final DoubleBinaryOperator binaryOperator) {
        this.symbol = symbol;
        this.binaryOperator = binaryOperator;
    }

    public String getSymbol() {
        return symbol;
    }

    @Override
    public double applyAsDouble(final double left, final double right) {
        return binaryOperator.applyAsDouble(left, right);
    }
}

public class CalculationFailedException extends RuntimeException {
    private static final long serialVersionUID = 6849565649585489467L;

    public CalculationFailedException() {
        super();
    }

    public CalculationFailedException(final String message) {
        super(message);
    }

    public CalculationFailedException(final Throwable cause) {
        super(cause);
    }

    public CalculationFailedException(final String message, final Throwable cause) {
        super(message, cause);
    }
}

public interface Calculator {
    public double calculate(final String input);
}

public class PostfixCalculator implements Calculator {
    private static final List<String> OPERATORS_LIST = Arrays.stream(Operator.values())
            .map(Operator::getSymbol)
            .collect(Collectors.toList());

    private static final Map<String, Operator> STRING_TO_OPERATOR_MAPPING = Arrays.stream(Operator.values())
            .collect(Collectors.toMap(op -> op.getSymbol(), op -> op));

    private final Stack<Double> numberStack = new Stack<>();

    @Override
    public double calculate(final String input) throws IllegalArgumentException {
        try {
            Arrays.stream(input.split(" ")).forEach(this::processElement);
            return safePop();
        } catch (CalculationFailedException | NumberFormatException ex) {
            throw new CalculationFailedException("Calculation has failed for expression: " + input);
        }
    }

    private void processElement(final String element) {
        Objects.requireNonNull(element);
        if (OPERATORS_LIST.contains(element)) {
            processOperator(STRING_TO_OPERATOR_MAPPING.get(element));
        }
        else {
            processNumber(Double.parseDouble(element));
        }
    }

    private void processOperator(final Operator operator) {
        double right = safePop();
        double left = safePop();
        numberStack.push(operator.applyAsDouble(left, right));
    }

    private void processNumber(final double number) {
        numberStack.push(number);
    }

    private double safePop() {
        if (numberStack.isEmpty()) {
            throw new CalculationFailedException();
        }
        return numberStack.pop();
    }
}

Code can be called for example with:

Calculator calculator = new PostfixCalculator();
double result = calculator.calculate("3 4 +");

Which will give 7.0 as result.

Another example:

Calculator calculator = new PostfixCalculator();
double result = calculator.calculate("5 1 2 + 4 * + 3 -");

Which gives 14.0 as result.

\$\endgroup\$
7
\$\begingroup\$

I like how the Functions have simplified the enum, but, you have put logic in the implementation class that belongs in the enum....

The code:

private static final List<String> OPERATORS_LIST = Arrays.stream(Operator.values())
        .map(Operator::getSymbol)
        .collect(Collectors.toList());

private static final Map<String, Operator> STRING_TO_OPERATOR_MAPPING = Arrays.stream(Operator.values())
        .collect(Collectors.toMap(op -> op.getSymbol(), op -> op));

should be part of the Operator enum.... and there should be a static method on the enum:

public static Operator operatorFor(String op) {
    return STRING_TO_OPERATOR_MAPPING.get(op);
}

Then, your calling class would have the code:

private void processElement(final String element) {
    Objects.requireNonNull(element);
    Operator op = Operator.operatorFor(element);
    if (op != null) {
        processOperator(op);
    }
    else {
        processNumber(Double.parseDouble(element));
    }
}

Also, you should be trapping the NumberFormatException closer to where it is thrown....

        processNumber(Double.parseDouble(element));

should be:

try {
    processNumber(Double.parseDouble(element));
} catch (NumberFormatException nfe) {
    throw new CalculationFailedException("Unable to parse value " + element + " as a double.", nfe);
}

That way, you can identify which value failed to parse.

\$\endgroup\$
  • \$\begingroup\$ Good answer, but you might not want to echo the value that failed to parse exactly as entered. If you do that, your application will be vulnerable to command injection if someone is trying to use its output on the command line again (for example, as an argument to another program). \$\endgroup\$ – Moshe Katz May 12 '14 at 21:06

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.