4
\$\begingroup\$

I have previously worked on this program in this question, and have gotten some very good answers and help! Now, I have done some more work on the program, and would love if you could give me honest feedback on what I need to change.

Main

package owl;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.Stack;

public class Main {

    public static void main(String[] args) {
        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");
        while (true) {
            try {
                String input = reader.readLine().toLowerCase();
                RPNcli.parse(input, stack);
            } catch (IOException e) {
                e.printStackTrace();
            }

        }
    }

}

This class just gets the input and passes it into the parse class. I think I am not using the Object Oriented part here correctly, with separating the program into the Main and RPNcli classes...

RPNcli

package owl;

import java.text.DecimalFormat;
import java.util.Arrays;
import java.util.HashSet;
import java.util.Set;
import java.util.Stack;

public class RPNcli {

    public static void parse(String input, Stack<Double> stack) {
        String[] parts = input.split(" ");
        for (int i = 0; i < parts.length; i++) {
            Double num = isNumber(parts[i]);
            if (num != null) {
                stack.push(num);
            } else
                operation(parts[i], stack);
        }
    }

    private static void operation(String input, Stack<Double> stack) {
        Set<String> simples = new HashSet<>(Arrays.asList("+", "-", "*", "/", "^"));
        Set<String> functions = new HashSet<>(Arrays.asList("sin", "cos", "tan", "asin", "acos", "atan", "sq"));
        Set<String> commands = new HashSet<>(Arrays.asList("p", "e", "c", "pop", "swap"));
        if (simples.contains(input)) {
            simple(input, stack);
        } else if (functions.contains(input)) {
            function(input, stack);
        } else if (commands.contains(input)) {
            command(input, stack);
        } else {
            System.out.println("ERROR: Invalid Command");
        }
    }

    private static void command(String input, Stack<Double> stack) {
        DecimalFormat df = new DecimalFormat("#,###.#########");
        switch (input) {
        case "e":
            System.exit(0);
            break;
        case "p":
            if (stack.size() > 0) {
                System.out.println(df.format(stack.peek()));
            } else
                System.out.println("ERROR: no stacks to peek");
            break;
        case "c":
            stack.clear();
            break;
        case "pop":
            if (stack.size() > 0) {
                stack.pop();
            } else
                System.out.println("ERROR: no stacks to pop");
            break;
        case "swap":
            if (stack.size() > 1) {
                double num2 = stack.pop();
                double num1 = stack.pop();
                stack.push(num2);
                stack.push(num1);
            } else
                System.out.println("ERROR: can't swap empty stack");
        }

    }

    private static void function(String input, Stack<Double> stack) {
        if (stack.size() > 0) {
            double num = stack.pop();
            double rads = Math.toRadians(num);
            double ans = 0;
            switch (input) {
            case "sq":
                ans = num * num;
            case "sin":
                ans = Math.sin(rads);
                break;
            case "cos":
                ans = Math.cos(rads);
                break;
            case "tan":
                ans = Math.tan(rads);
                break;
            case "asin":
                ans = Math.asin(rads);
                break;
            case "acos":
                ans = Math.acos(rads);
                break;
            case "atan":
                ans = Math.atan(rads);
                break;
            }
            stack.push(ans);
        } else
            System.out.println("ERROR: can't operate on empty stack");
    }

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

    private static Double isNumber(String input) {
        try {
            double num = Double.parseDouble(input);
            return num;
        } catch (NumberFormatException n) {
            return null;
        }
    }
}

Here the input is parsed and all of the operations happen, based on the data parsed. I think maybe I could split this class into two classes called "Parser" and "Calculator". Please give me your opinion on this, I really want to learn how to do this better.

P.S. If you'd rather look at the code on GitHub or clone to run it, here is the GitHub page.

\$\endgroup\$
3
\$\begingroup\$

As you already said, it is a good idea separate commands from maths operations with the creation of two classes , I made two classes to give you a rough idea. In your Main class you can use Try with resources construct with your buffer, below the code

Main.java

public class Main {

    public static void main(String[] args) {
        try (BufferedReader reader = new BufferedReader(new InputStreamReader(System.in))) {
            System.out.println("JavaRPN: Input numbers and operands separated by newline or space");
            RpnCli cli = new RpnCli();
            while (true) {
                String input = reader.readLine().toLowerCase();
                cli.parse(input);
            }
        } catch (IOException e) {
            e.printStackTrace();
        }
    }
}

You can see I created a RpnCli instance to avoid static method (I put static just the set of commands for possible future purposes). I updated the RpnCli class in the followind way:

RpnCli.java

import java.util.ArrayDeque;
import java.util.Arrays;
import java.util.Deque;
import java.util.HashSet;
import java.util.Set;

public class RpnCli {
    private final static Set<String> commands = new HashSet<>(Arrays.asList("p", "e", "c", "pop", "swap"));
    private Deque<Double> stack;
    private Calculator calculator;

    public RpnCli() {
        this.stack = new ArrayDeque<>();
        this.calculator = new Calculator(stack);
    }

    public void parse(String input) {
        String[] parts = input.split(" ");
        for (int i = 0; i < parts.length; i++) {
            try {
                Double num = Double.parseDouble(input);
                stack.push(num);
            } catch (NumberFormatException ex) { 
                operation(parts[i]);
            }
        }
    }

    private void operation(String input) {
        if (calculator.contains(input)) {
            calculator.calculate(input);
        } else {
            if (commands.contains(input)) {
                command(input);
            } else {
                System.out.println("ERROR: Invalid Command");
            }
        }
    }

    private void command(String input) { /*omitted for brevity*/ }
}

I divided math operations from command operations, so I created a new class Calculator for math operations that in its constructor has as its parameter the stack you use for all operations. You can see I used Deque class for variable stack because Stack java class is less complete compared to it. I also modified your parse method so if the input is not a double value will be parsed by operation method as you already made in your code. I added a new class called Calculator, below the code:

Calculator.java

import java.util.Arrays;
import java.util.Deque;
import java.util.HashSet;
import java.util.Set;

public class Calculator {
    private final static Set<String> simples = new HashSet<>(Arrays.asList("+", "-", "*", "/", "^"));
    private final static Set<String> functions = new HashSet<>(Arrays.asList("sin", "cos", "tan", "asin", "acos", "atan", "sq"));
    private Deque<Double> stack;

    public Calculator(Deque<Double> stack) {
        this.stack = stack;
    }

    public boolean contains(String input) {
        return simples.contains(input) || functions.contains(input);
    }

    public void calculate(String input) {
        if (simples.contains(input)) {
            simple(input);
        } else {
            function(input);
        }
    }

    private void function(String input) { /*omitted for brevity*/ }

    private void simple(String input) { /*omitted for brevity*/ }
}

The most important thing is this class is the field Stack passed by the RpnCli class: it will no more appear as a parameter in the methods like the methods of the containing class RpnCli and so there is no more need of static methods.

\$\endgroup\$
  • \$\begingroup\$ I know I am late, but I would like to take a look at the "contains" method. The calculate method decides if we are using simples if contains returns true. But contains will always return true if there is a function or a simple... \$\endgroup\$ – ViceroyFaust Nov 2 at 23:11
  • \$\begingroup\$ And when I try to use my function and simple methods, it tells me that I "cannot make a static reference to a non static field. \$\endgroup\$ – ViceroyFaust Nov 2 at 23:17
  • \$\begingroup\$ @ViceroyFaust, the contains method checks if your token is a simple or a function , if you define an instance calc you can call like if(calc.contains()) {calc.calculate()}. The error is due to the fact you have a static method and you are calling inside of it an instance calc defined outside it, so you have to remove static from method. \$\endgroup\$ – dariosicily Nov 4 at 11:13
4
\$\begingroup\$

Bugs

The input to asin, acos, and atan is not radians. That is the output unit. sin(30°) == sin(π/2) == 0.5. Converting the input to radians makes sense. asin(0.5) == π/2 == 30°. You should call toDegrees on the output, not toRadians on the input.

Fall-through causes "sq" to produce sin(rads) instead:

        case "sq":
            ans = num * num;
        case "sin":
            ans = Math.sin(rads);
            break;
\$\endgroup\$
  • \$\begingroup\$ Thank you so much! Somehow I did not realise I forgot to put a break on sq. Also, but isn't 30 degrees to pi/2 going to produce the same output? \$\endgroup\$ – ViceroyFaust Oct 12 at 20:31
  • \$\begingroup\$ 30° is the number 30.0; π/2 is the number 1.57079632679.... Those are quite different floating point values. Math.toRadians(30.0) = 1.57079..., Math.sin(1.57979...) = 0.5, Math.asin(0.5) = 1.57079..., and Math.toDegrees(1.57079...) = 30.0. \$\endgroup\$ – AJNeufeld Oct 12 at 22:01
  • \$\begingroup\$ Oh, thank you for the clarification! I will fix that asap. @AJNeufeld \$\endgroup\$ – ViceroyFaust Oct 13 at 0:02

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.