patternjavaMinor
Executing an operator in an RPN evaluator
Viewed 0 times
evaluatorexecutingoperatorrpn
Problem
Is there any way to take this chunk of an else if and make it its own method? I would need the values of valid and throwLine() to be done in the method containing this if else statement, but I'm around 45 lines in my method which is 15 more than I'm allowed to have for simplicity.
else if(isOperator(readIn))
{
System.out.print(readIn);
if(myStack.isEmpty())
valid = false;
else
op2 = (Fraction)myStack.pop();
if(myStack.isEmpty())
{
throwLine(readIn);
valid = false;
}
else
{
runTheOperator(op2, op1, readIn);
readIn = stdin.next();
}Solution
As @DavidHarkness points out, you didn't provide enough context for this question to make sense. However, I see that this code comes from a
The root problem is that your method has no clear purpose, and tries to do many things:
As I mentioned in my other answer, that's not a good way to write an RPN calculator.
The main loop of an RPN evaluator should probably look something like this:
That's all! To summarize:
doTypeCheck() method, which is at the heart of your RPN calculator implementation. I'll reproduce that method here, along with its call site:private void doTypeCheck(Fraction op1, Fraction op2)
{
Fraction answer = null;
String readIn = "";
boolean valid = true;
readIn = stdin.next();
while(!readIn.equals("#") && valid == true)
{
if(!isOperator(readIn) && isOperand(readIn))
{
processOperand(readIn);
readIn = stdin.next();
}
else if(isOperator(readIn))
{
System.out.print(readIn);
if(myStack.isEmpty())
valid = false;
else
op2 = (Fraction)myStack.pop();
if(myStack.isEmpty())
{
valid = false;
throwLine(readIn);
}
else
{
runTheOperator(op2, op1, readIn);
readIn = stdin.next();
}
}
else
{
System.out.print(readIn);
valid = false;
throwLine(readIn);
}
}
System.out.println();
if(myStack.isEmpty())
valid = false;
else
answer = (Fraction)myStack.pop();
if(!myStack.isEmpty())
valid = false;
checkMessageValid(valid, answer);
}
private void runOnce(int count)
{
Fraction op1 = null;
Fraction op2 = null;
clear(myStack, myQueue);
System.out.print("Expression " + count++ + " is: ");
doTypeCheck(op1, op2);
}The root problem is that your method has no clear purpose, and tries to do many things:
- Read tokens from the input stream, and decide whether they are operands or operators
- If it sees invalid input, discard the rest of the input and display an error
- Pop an operand from the stack, with error handling
- Feed two operands to the calculating function
- Check that the stack is empty afterwards
- Display the answer
As I mentioned in my other answer, that's not a good way to write an RPN calculator.
The main loop of an RPN evaluator should probably look something like this:
try {
while (in.hasNext()) {
String token = in.next();
RpnOperator op = RpnOperator.forSymbol(token);
if (op != null) {
op.operate(calcStack);
} else {
calcStack.push(new Fraction(token));
}
}
System.out.println(calcStack.peek());
} catch (EmptyStackException emptyStack) {
System.out.println("Error: empty stack");
}That's all! To summarize:
- Let the operators manipulate the stack. The main loop just needs to dispatch to the appropriate operators.
- Use exceptions for error handling.
Code Snippets
private void doTypeCheck(Fraction op1, Fraction op2)
{
Fraction answer = null;
String readIn = "";
boolean valid = true;
readIn = stdin.next();
while(!readIn.equals("#") && valid == true)
{
if(!isOperator(readIn) && isOperand(readIn))
{
processOperand(readIn);
readIn = stdin.next();
}
else if(isOperator(readIn))
{
System.out.print(readIn);
if(myStack.isEmpty())
valid = false;
else
op2 = (Fraction)myStack.pop();
if(myStack.isEmpty())
{
valid = false;
throwLine(readIn);
}
else
{
runTheOperator(op2, op1, readIn);
readIn = stdin.next();
}
}
else
{
System.out.print(readIn);
valid = false;
throwLine(readIn);
}
}
System.out.println();
if(myStack.isEmpty())
valid = false;
else
answer = (Fraction)myStack.pop();
if(!myStack.isEmpty())
valid = false;
checkMessageValid(valid, answer);
}
private void runOnce(int count)
{
Fraction op1 = null;
Fraction op2 = null;
clear(myStack, myQueue);
System.out.print("Expression " + count++ + " is: ");
doTypeCheck(op1, op2);
}try {
while (in.hasNext()) {
String token = in.next();
RpnOperator op = RpnOperator.forSymbol(token);
if (op != null) {
op.operate(calcStack);
} else {
calcStack.push(new Fraction(token));
}
}
System.out.println(calcStack.peek());
} catch (EmptyStackException emptyStack) {
System.out.println("Error: empty stack");
}Context
StackExchange Code Review Q#44023, answer score: 6
Revisions (0)
No revisions yet.