5
\$\begingroup\$

I created a highly effective Reverse Polish Notation calculator using Java 8, but am uncertain if there are any better ways to handle the problem. Feedback on any evident taboos and bugs is greatly appreciated!

public final class RPN {

    /**
     * Removes all characters that are not operators, whitespace, or digits
     *
     * @param expr the expression to parse
     * @return the parsed expression
     */
    private static String parseExpression(String expr) {
        String parsedExpr = expr.replaceAll("[^\\^\\*\\+\\-\\d/\\s]", "");
        String trimmedExpr = parsedExpr.replaceAll("\\s+", " ");
        return trimmedExpr;
    }

    /**
     * Computes the outcome of a given expression in Reverse Polish Notation
     *
     * @param expr the expression to compute
     */
    public static void compute(String expr) throws
            ArithmeticException,
            EmptyStackException {
        String validExpr = parseExpression(expr);
        Stack<Double> stack = new Stack<>();

        System.out.println(validExpr);
        System.out.println("Input\tOperation\tStack after");

        for (String token : validExpr.split("\\s")) {
            System.out.print(token + "\t");

            double secondOperand = 0.0;
            double firstOperand = 0.0;

            switch (token) {
                case "+":
                    System.out.print("Operate\t\t");

                    secondOperand = stack.pop();
                    firstOperand = stack.pop();

                    stack.push(firstOperand + secondOperand);
                    break;
                case "-":
                    System.out.print("Operate\t\t");

                    secondOperand = stack.pop();
                    firstOperand = stack.pop();

                    stack.push(firstOperand - secondOperand);
                    break;
                case "*":
                    System.out.print("Operate\t\t");

                    secondOperand = stack.pop();
                    firstOperand = stack.pop();

                    stack.push(firstOperand * secondOperand);
                    break;
                case "/":
                    System.out.print("Operate\t\t");

                    secondOperand = stack.pop();
                    firstOperand = stack.pop();

                    if (secondOperand == 0.0) {
                        throw new ArithmeticException("Cannot divide by zero!");
                    }

                    stack.push(firstOperand / secondOperand);
                    break;
                case "^":
                    System.out.print("Operate\t\t");

                    secondOperand = stack.pop();
                    firstOperand = stack.pop();

                    stack.push(Math.pow(firstOperand, secondOperand));
                    break;
                default:
                    System.out.print("Push\t\t");
                    stack.push(Double.parseDouble(token));
                    break;
            }

            System.out.println(stack);
        }

        System.out.println("Final Answer: " + stack.pop());
    }

    /**
     * What runs the program
     *
     * @param args the command line arguments
     */
    public static void main(String[] args) {
        try {
            String expr = Arrays.toString(args);
            System.out.println(expr);
            compute(expr);
        } catch (Throwable err) {
            System.out.println(err.getMessage());
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Mast Feb 18 '16 at 22:56
  • \$\begingroup\$ Alright thanks! I was just trying to formulate a way to show @200_success how the code broke. \$\endgroup\$ – T145 Feb 18 '16 at 23:02
5
\$\begingroup\$

Your parseExpression() function isn't parsing anything. Rather, it sanitizes the string. Sanitizing input is a rather risky business. For example, any decimal points will be discarded! What if I try to use ! as the factorial operator? That would be silently ignored as well. If you can't handle the input in a reasonable way, it should be an error.

It's odd that you convert args from an array to a string, then split it up again. You could either use args directly, or split just args[0].

Catching Throwable is probably a bad idea. Anything that is not an Exception is generally not meant to be caught and handled.

It's actually not true that you cannot divide by zero. That's an error only when performing integer arithmetic. Why not let Double.POSITIVE_INFINITY be a valid result?

The code would be simpler if you didn't assign to variables firstOperand and secondOperand. I'd write it more like this:

import java.util.*;

public class RPN {

    /**
     * Computes the outcome of a given expression in Reverse Polish Notation
     *
     * @param expr the expression to compute
     */
    public static void compute(String expr) throws
            ArithmeticException,
            EmptyStackException {
        Stack<Double> stack = new Stack<>();

        System.out.println(expr);
        System.out.println("Input\tOperation\tStack after");

        for (String token : expr.split("\\s+")) {
            System.out.print(token + "\t");
            switch (token) {
                case "+":
                    System.out.print("Operate\t\t");
                    stack.push(stack.pop() + stack.pop());
                    break;
                case "-":
                    System.out.print("Operate\t\t");
                    stack.push(-stack.pop() + stack.pop());
                    break;
                case "*":
                    System.out.print("Operate\t\t");
                    stack.push(stack.pop() * stack.pop());
                    break;
                case "/":
                    System.out.print("Operate\t\t");
                    double divisor = stack.pop();
                    stack.push(stack.pop() / divisor);
                    break;
                case "^":
                    System.out.print("Operate\t\t");
                    double exponent = stack.pop();
                    stack.push(Math.pow(stack.pop(), exponent));
                    break;
                default:
                    System.out.print("Push\t\t");
                    stack.push(Double.parseDouble(token));
                    break;
            }

            System.out.println(stack);
        }

        System.out.println("Final Answer: " + stack.pop());
    }

    /**
     * Runs the calculation for the RPN expression in args[0].
     */
    public static void main(String[] args) {
        try {
            compute(args[0]);
        } catch (Exception err) {
            System.out.println(err.getMessage());
        }
    }
}

Sample run:

$ java RPN '3 5 +'
3 5 +
Input   Operation   Stack after
3   Push        [3.0]
5   Push        [3.0, 5.0]
+   Operate     [8.0]
Final Answer: 8.0
\$\endgroup\$
  • \$\begingroup\$ Your code breaks when it goes into the switch statement, and doesn't generate a final answer. \$\endgroup\$ – T145 Feb 18 '16 at 23:00
  • \$\begingroup\$ Do you have an example expression that breaks it? \$\endgroup\$ – 200_success Feb 18 '16 at 23:04
  • \$\begingroup\$ Just do something like 3 5 +. I get this right after the "Input, Operate, Stack" header: [3, Push For input string: "[3," \$\endgroup\$ – T145 Feb 18 '16 at 23:07
  • \$\begingroup\$ I don't know what you're talking about; it works just fine. \$\endgroup\$ – 200_success Feb 18 '16 at 23:11
  • \$\begingroup\$ I didn't know you had made changes to the rest of the class; I just plugged in the function directly to the program! After changing the input mechanism to be String[] as you suggested, it works just fine. Thanks a lot! \$\endgroup\$ – T145 Feb 18 '16 at 23:16

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.