6
\$\begingroup\$

I have found that there is a distinct lack of an RPN calculator available to me, so I decided to make my own. I am still adding new functions to it, but as of now, I would like to refactor my code.

Main of JavaRPN

Because this is a small problem, and I haven't made a GUI for it, I only use one class.

package owl;

import java.io.BufferedReader;
import java.io.InputStreamReader;
import java.text.DecimalFormat;
import java.util.Stack;

public class Main {

    public static void main(String[] args) {
        try {
            BufferedReader reader = new BufferedReader(new InputStreamReader(System.in));
            Stack<Double> stack = new Stack<Double>();
            System.out.println("JavaRPN: Input numbers and operands separated by newline or space");
            DecimalFormat df = new DecimalFormat("#,###.#########");
            while (true) {
                String input = reader.readLine();
                String[] inputs = input.split(" ");
                for (int i = 0; i < inputs.length; i++) {
                    if (isNumber(inputs[i])) {
                        stack.push(Double.parseDouble(inputs[i]));
                        continue;
                    }
                    if (inputs[i].equals("e") || inputs[i].equals("p") || inputs[i].equals("c")) {
                        commands(inputs[i], stack, df);
                    } else if (inputs[i].equals("sq") || inputs[i].equals("sin") || inputs[i].equals("cos")
                            || inputs[i].equals("tan") || inputs[i].equals("asin") || inputs[i].equals("acos")
                            || inputs[i].equals("atan")) {
                        function(inputs[i], stack);
                    } else if (inputs[i].equals("+") || inputs[i].equals("-") || inputs[i].equals("*")
                            || inputs[i].equals("/") || inputs[i].equals("^")) {
                        operator(stack, inputs[i]);
                    } else {
                        System.out.println("ERROR: Invalid input");
                    }
                }
            }
        } catch (

        Exception e) {
            e.printStackTrace();
        }
    }

    private static void commands(String input, Stack<Double> stack, DecimalFormat df) {
        switch (input) {
        case "e":
            System.exit(0);
            ;
            break;
        case "p":
            if (stack.size() > 0) {
                System.out.println(df.format(stack.peek()));
                break;
            } else {
                System.out.println("ERROR: All Stacks Empty");
                break;
            }
        case "c":
            stack.clear();
            break;
        }

    }

    private static void function(String string, Stack<Double> stack) {
        if (stack.size() > 0) {
            double num = stack.pop();
            switch (string) {
            case "sq":
                stack.push(num * num);
                break;
            case "sin":
                stack.push(Math.sin(Math.toRadians(num)));
                break;
            case "cos":
                stack.push(Math.cos(Math.toRadians(num)));
                break;
            case "tan":
                stack.push(Math.tan(Math.toRadians(num)));
                break;
            case "asin":
                stack.push(Math.asin(Math.toRadians(num)));
                break;
            case "acos":
                stack.push(Math.acos(Math.toRadians(num)));
                break;
            case "atan":
                stack.push(Math.atan(Math.toRadians(num)));
                break;
            }
        }
    }

    private static void operator(Stack<Double> stack, String input) {
        if (stack.size() > 1) {
            double num2 = stack.pop();
            double num1 = stack.pop();
            switch (input) {
            case "+":
                stack.push(num1 + num2);
                break;
            case "-":
                stack.push(num1 - num2);
                break;
            case "*":
                stack.push(num1 * num2);
                break;
            case "/":
                stack.push(num1 / num2);
                break;
            case "^":
                stack.push(Math.pow(num1, num2));
                break;
            }
        } else {
            System.out.println("ERROR: Can't operate on an empty stack");
        }
    }

    private static boolean isNumber(String input) {
        try {
            Double.parseDouble(input);
            return true;
        } catch (Exception e) {
            return false;
        }
    }

}

Anyone familiar with RPN calculators and the UNIX system is probably familiar with the built in dc calculator. The calculator parses input very similar to that old program, but has support for decimals unlike the old one (or at least I couldn't find a way to use decimals in it).

I am aware theat I am clearly being redundant on the line where the code checks the input before executing a method, which checks for the same exact method. I attempted to refactor that before, but I couldn't figure out for the life of my how to do it more efficiently without breaking my current code.

I frequently update this project on my github: https://github.com/ViceroyFaust/JavaRPN/tree/Refactoring ^^^ I have made updates, please tell me whether I've improved or made the problem worse ^_^

\$\endgroup\$
  • \$\begingroup\$ Just a suggestion: instead of doing inputs[i].equals(...) a bunch of times, try Arrays.asList("e", "p", "c").contains(inputs[i]). It basically just initialises a new list, then checks if it contains the string using the contains method, which is only available in lists (hence why we have to use asList instead of String[]). \$\endgroup\$ – Geza Kerecsenyi Aug 25 at 17:34
  • \$\begingroup\$ @GezaKerecsenyi That makes things much simpler. But, does it theoretically hinder or increase the effiency of the program? \$\endgroup\$ – ViceroyFaust Aug 25 at 17:39
  • \$\begingroup\$ Also, line 49 is just an unnecessary semicolon. \$\endgroup\$ – Geza Kerecsenyi Aug 25 at 17:40
  • \$\begingroup\$ Regarding your comment, no, it shouldn't. While I've not tested it (and I'm also a beginner to Java), they both stop as soon as they detect something. The only thing that could possibly be slower is initialising the array, but I'm fairly certain it's negligible. \$\endgroup\$ – Geza Kerecsenyi Aug 25 at 17:41
  • 2
    \$\begingroup\$ In fact, for a very small number of elements, linear lookup can be faster than the overhead of a hash calculation and a set lookup. But whichever is faster is of no concern as long as we are looking at sub-millisecond optimizations in relation to direct user input. And at the OP: please don't concern yourself with speed (yet). Learn how to write well-structured and maintainable software which is fun to work with first. (You have come to the right place for that ;-)) \$\endgroup\$ – mtj Aug 26 at 11:40
4
\$\begingroup\$

In those lines doing equality checks, you write inputs[i] repeatedly. This has two main problems:

  • It's likely bulkier than necessary
  • If you ever need to change from inputs[i] to something else, you're needing to change it roughly 17 places! That's not ideal.

You can fix at least the second problem by just creating an intermediate variable:

for (int i = 0; i < inputs.length; i++) {
    String curInput = inputs[i];  // Create an intermediate

    if (isNumber(curInput)) {  // Then use it everywhere
        stack.push(Double.parseDouble(curInput));
        continue;
    }
    if (curInput.equals("e") || curInput.equals("p") || curInput.equals("c")) {
    . . . 

Now, if you need to change what defines the current input, you only need to make the change in one place instead of 10+ places. This doesn't really help bulk though as curInput is only one character shorter. It can certainly help sometimes though.

The main problem with that whole bit though is you're writing .equals(. . .) all over the place. Whenever you need to check one variable against several inputs, consider using a Set (like a HashSet):

// Create a set containing the input to check against.
// The "asList" part is just a shortcut so you don't need to call "add" a whole bunch of times
Set<String> functionInputs = new HashSet<>(Arrays.asList("sq", "sin", "cos", "tan", "asin", "acos", "atan"));

. . .

} else if (functionInputs.contains(curInput)) {  // Now it's much shorter and cleaner
    function(inputs[i], stack);

Then the same can be done for the other types of checks. Create a set holding all the different types to check against, then use contains to check if the input is in that set.

For an explanation of the asList shortcut I'm using, see here.


} catch (

Exception e) {
    e.printStackTrace();
}

I don't like a couple things here:

  • You're using odd formatting. I don't see why this should be split over a few lines.
  • You should not be blindly catching Exception. You're printing the stack trace when an error has occurred, so you aren't silencing helpful errors, but you are potentially catching errors you shouldn't be. It seems like your intent there is to catch a NumberFormatException thrown by parseDouble. If that's the case, catch that specifically:

    } catch (NumberFormatException e) {
        e.printStackTrace();
    }
    

I'd still restructure this though. It would probably be a better idea to pre-check and pre-process the input before running through it in the main loop. I'd pre-parse all the inputs in a separate function. That way, you can entirely remove the try from the loop in main and make the code cleaner. main is huge and is doing a lot. I would try to move a lot of that functionality out into separate, smaller functions. That will make each piece of code much clearer.


Back onto the topic of duplication, look at this code and think about if it has unnecessary duplication:

double num = stack.pop();
switch (string) {
case "sq":
    stack.push(num * num);
    break;
case "sin":
    stack.push(Math.sin(Math.toRadians(num)));
    break;
case "cos":
    stack.push(Math.cos(Math.toRadians(num)));
    break;
case "tan":
    stack.push(Math.tan(Math.toRadians(num)));
    break;
case "asin":
    stack.push(Math.asin(Math.toRadians(num)));
    break;
case "acos":
    stack.push(Math.acos(Math.toRadians(num)));
    break;
case "atan":
    stack.push(Math.atan(Math.toRadians(num)));
    break;
}

How many times do you call toRadians in that code? What if you add more functions to deal with? Convert the number, then check against that:

double num = stack.pop();
double rads = Math.toRadians(num);  // Store radians here
switch (string) {
case "sq":
    stack.push(num * num);
    break;
case "sin":
    stack.push(Math.sin(rads));
    break;
case "cos":
    stack.push(Math.cos(rads));
    break;
case "tan":
    stack.push(Math.tan(rads));
    break;
case "asin":
    stack.push(Math.asin(rads));
    break;
case "acos":
    stack.push(Math.acos(rads));
    break;
case "atan":
    stack.push(Math.atan(rads));
    break;
}

Note how you have stack.push over and over as well. You could get rid of that duplication by calling it after the switch:

double num = stack.pop();
double rads = Math.toRadians(num);  // Store radians here

Double answer = null;  // 
switch (string) {
case "sq":
    answer = num * num;
    break;
case "sin":
    answer = Math.sin(rads);
    break;
case "cos":
    answer = Math.cos(rads);
    break;
case "tan":
    answer = Math.tan(rads);
    break;
case "asin":
    answer = Math.asin(rads);
    break;
case "acos":
    answer = Math.acos(rads);
    break;
case "atan":
    answer = Math.atan(rads);
    break;
}

if (answer) {
    stack.push(answer);
}

Now if you ever change how the stack you're using works, you don't need to make multiple changes. This still isn't great though. Now I have answer = duplicated. You could make use of some semi-advanced functional techniques and store the functions in a Map, and dispatch on it:

import java.util.function.DoubleUnaryOperator;
import java.util.Map;
import java.util.HashMap;

Map<String, DoubleUnaryOperator> nameToFunc = new HashMap<>();
nameToFunc.put("sin", Math::sin);
nameToFunc.put("cos", Math::cos);
nameToFunc.put("tan", Math::tan);
. . . // And the rest of the mappings

Then

private static void function(String string, Stack<Double> stack) {
    if (stack.size() > 0) {
        double num = stack.pop();
        DoubleUnaryOperator f = nameToFunc.get(string);  // Get the func
        Double answer = f.applyAsDouble(Math.toRadians(num));  // Will be null if it's a bad string

        if (answer != null) {
            stack.push(answer);
        }
    }
}

For simplicity, I ignored the "sq" case though. Since it uses non-radian input, it's a special case.

The advantage of using this is, because it uses the same strings as the Set suggestion I made at the top, you can change the definition of functionInputs to be based on nameToFunc:

Set<String> functionInputs = nameToFunc.keySet();

Now, if you add more function names to handle, you only need to update nameToFunc and they'll both be updated. You could also just avoid functionInputs altogether and just use nameToFunc. You could change your checks to:

} else if (nameToFunc.get(curInput)) {  // "get" returns null (falsey) on a bad lookup
    function(inputs[i], stack);

You write:

if (isNumber(curInput)) {
    stack.push(Double.parseDouble(curInput));
    continue;
}

There's one performance-related issue with this: your isNumber already calls parseDouble. You could avoid this by changing isNumber:

private static Double maybeParse(String input) {
    try {
        return Double.parseDouble(input);

    } catch (NumberFormatException e) {  // Only catch what you intend on catching! 
        return null;  // Return null on failure
    }
}

Then you can do:

Double maybeN = maybeParse(curInput);
if (maybeN != null) {
    stack.push(maybeN);
    continue;
}

Instead of returning a null on a bad parse, you could also make use of Optional here. What I'm showing in maybeParse is basically the Optional pattern, minus the use of the standard wrapper class. As long as you document that null may be returned, this should be fine. Optional is nice though in that it's self-documenting.

The use of Double will cause a little overhead due to the boxing/unboxing of the double. I expect the cost to be less than it is to parse the string twice though.


Hopefully this gives you some good ideas. Good luck!

\$\endgroup\$
  • \$\begingroup\$ The Catch part isn't my formatting, I messed up that part while pasting, oops! \$\endgroup\$ – ViceroyFaust Aug 26 at 1:21
  • \$\begingroup\$ Instead of defining the functions as DoubleUnaryOperators I would define them as Consumer<Stack<Double>>. This way the function would be responsible for knowing how many parameters it takes and the '+', '-', '*', etc functions could be defined in the same structure. \$\endgroup\$ – TorbenPutkonen Aug 26 at 7:17
  • \$\begingroup\$ @TorbenPutkonen That's a good point. I'll see if I have time to add something after work. \$\endgroup\$ – Carcigenicate Aug 26 at 11:07
  • \$\begingroup\$ I can't actually return a null as a double, what do? \$\endgroup\$ – ViceroyFaust Aug 27 at 14:34
  • \$\begingroup\$ @ViceroyFaust If you're referring to my maybeParse, note that the return type is Double, not double. The first is an object that may contain null, the latter is a primitive that can't contain null. \$\endgroup\$ – Carcigenicate Aug 27 at 15:22
4
\$\begingroup\$

Putting all code into one class makes the program very complicated. Refactor the code into classes where each class performs one task. This is called Single Responsibility Principle

The core of the RpnEgine is the following function. I've used Deque as it performs better than the synchronized Stack.

public void process(String ... input) {
    for (String s: input) {
        final Consumer<Deque<Double>> func = FUNCTIONS.get(s);
        if (func != null) {
            func.accept(stack);
        } else {
            stack.push(Double.valueOf(s));
        }
    }
}

The mathematical operations and commands can then be defined as lambdas or standalone classes. These are a bit ugly, as the operator order is reversed hen read from the stack. You'll notice this code repeats the pushing and popping a lot, so it might be a good idea to refactor them to a common class that check stack size, pops the operands, delegates them to a BiFunction and pushes the result.

It also introduces great flexibility, as implementing a function that calculates the sum of whatever is in the stack becomes trivial.

static {
    FUNCTIONS.put("+", (d) -> d.push(d.pop() + d.pop()));
    FUNCTIONS.put("-", (d) -> d.push((- d.pop()) + d.pop()));
    FUNCTIONS.put("/", (d) -> d.push(1.0 / (d.pop() / d.pop())));
    FUNCTIONS.put("sum", new Sum());
}

Do not put the input parsing to the same class. Create a class named RpnCli that reads input and passes it to the RpnEngine.

\$\endgroup\$
  • 1
    \$\begingroup\$ What does the Deque do which Stack doesn't? Also, I am not sure how to use BiFunction, so I am kind of confused what is happening in that static {...} method. \$\endgroup\$ – ViceroyFaust Aug 27 at 13:43
  • 1
    \$\begingroup\$ And I am just in general confused where did you get Consumer, FUNCTIONs and all that from... \$\endgroup\$ – ViceroyFaust Aug 27 at 14:47
  • \$\begingroup\$ The static block initializes a HashMap<String, Consumer<Deque<Double>>> using lambda expressions and one plain old Java object. If you're just learning Java, lambdas are probably not the first things on the curriculum. \$\endgroup\$ – TorbenPutkonen Aug 27 at 18:19

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.