Problem
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.
Solution
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.
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;