Cleaning up Reverse Polish Notation evaluator

I was wondering if anyone had some general clean up advice on my code. I believe that more helper functions would be my case but I could be wrong. 30 lines maximum per method is my rule. I can't seem to figure out how to clean this up more, though.

Sample Input:

(1/3) (1/5) - (40/1) * #

(2/3) B * #

Sample Output


Expression 1 is: (1/3)(1/5)-(40/1)*


The value is: (16/3)


Intermediate results: (2/15)(16/3)


Expression 2 is: (2/3)B


Invalid Expression


Intermediate results:

```
import java.util.Scanner;

public class RpnEvaluator
{
private final int MAX_TOKEN = 100;
private Scanner stdin = new Scanner(System.in);
private Queue myQueue = new Queue(MAX_TOKEN);
private Stack myStack = new Stack(MAX_TOKEN);

public void run() throws java.io.IOException
{
int count = 1;
Fraction myInterMid;
while(stdin.hasNext())
{
runOnce(count++);
System.out.print("Intermediate results: ");
while(!myQueue.isEmpty())
{
myInterMid = (Fraction)myQueue.remove();
System.out.print(myInterMid.toString());
}
System.out.println();
clear(myStack, myQueue);
}
System.out.println("Normal Termination of Program 3.");
}

private boolean isOperator(String input)
{
String[] oprtr = {"+", "-", "*"};
for(String choice: oprtr)
if(choice.equals(input))
return true;
return false;
}

private boolean isOperand(String input)
{
if(input.charAt(0) == '(')
return true;
return false;
}

private Fraction runOperation(String choice, Fraction op2, Fraction op1)
{
Fraction newFract = new Fraction();
switch (choice)
{
case "*":
newFract = new Fraction(op1.times(op2));
break;
case "+":
newFract = new Fraction(op1.plus(op2));
break;
case "-":
newFract = new Fr

Reverse Polish Notation does not need parentheses, so that should actually be invalid input and should not be checked.

To determine if something is an operand you should be able to use stdin.hasNextInt(). If that is false, then you should be able to use stdin.next() to get whatever the operator is. This could greatly simplify your type checking.

Instead of terminating input with # you should just check for newlines.

I'm not sure what the purpose of having myQueue is. You should only need a stack for processing RPN.

I would recommend reading up on RPN to understand it more (the Wikipedia page has an algorithm for interpreting RPN) and see if that gives you ideas on how you might rethink your approach.

Edit: Assuming that the input format and output values are requirements, there are several things you can do to improve the code.

• MAX_TOKEN should be private static final.

• You should rename your methods to better describe what they do. runOnce() is called in a loop, so it is not called once, and it doesn't explain what it does. I would recommend something like processNextCalculation(). doTypeCheck(), throwLine() and checkMessageValid() should also be renamed.

• You should move the output in checkMessageValid() to be in the same place as where you output the intermediate results. You will need to change some return values to propagate the results upward.

• isOperand() can be reduced to return input.charAt(0) == '(';

• In run() you don't use myInterMid elsewhere so you can remove references to it and simply System.out.println(myQueue.remove());

• You should try to be consistent about using my as a prefix. Typically, it implies that the variable is an instance member variable, but you also use it for variables within methods. That is definitely confusing.

• I don't know what is in your Fraction class, but I imagine that the times(), plus(), and minus() methods are returning Fractions, so you don't need to new one up from the result.

• You don't need to declare op1 and op2 in runOnce(), since you can just pass two nulls to doTypeCheck(). For that matter, you don't need to even have parameters for doTypeCheck(). Once you've done that, you should bring the rest of the method up to run() so you can keep your text output in the same place.

• Since you can't have both isOperator() and isOperand() be true, you don't need to check both in your first if condition in doTypeCheck().

• You don't need to check myStack.isEmpty() twice, you should be able to check if myStack.size() is less than two. If your implementation of Stack doesn't have a size() method, well...I feel bad for you.

• Whenever myStack.isEmpty() is true, you can break out of the loop and your failure case will still show correctly.

• You only need one Fraction in processOperand() and you can initialize it on the same line as you declare it.

StackExchange Code Review Q#43984, answer score: 8