Note that the JavaDoc for Stack says to use Deque Stack says to use Deque instead.
Note that the JavaDoc for Stack says to use Deque instead.
Note that the JavaDoc for Stack says to use Deque instead.
public class EvaluateReversePolishNotation {
As a general rule, name classes and objects as nouns. Verbs are used for methods. I personally like PostfixCalculator, but if you want these words, consider:
public class ReversePolishNotationEvaluator {
Now it's a noun.
HashSet<String> ops = new HashSet<>(Arrays.asList("+","-","*","/"));
I would mark down for this when evaluating code.
Set<String> ops = new HashSet<>(Arrays.asList("+", "-", "*", "/"));
Unless you are specifically using some method that only exists in that specific implementation, I would always recommend using the interface as the type.
I prefer a little whitespace in my lists.
Note that you don't actually need this.
HashSet<String> ops = new HashSet<>(Arrays.asList("+","-","*","/"));
for (int i=0; i<tokens.length; i++) {
if (!ops.contains(tokens[i])) {
stk.push(Integer.parseInt(tokens[i]));
}
else {
int secondOperand = stk.pop();
int firstOperand = stk.pop();
switch (tokens[i]) {
case "+":
stk.push(firstOperand+secondOperand);
break;
case "-":
stk.push(firstOperand-secondOperand);
break;
case "*":
stk.push(firstOperand*secondOperand);
break;
case "/":
if (secondOperand == 0) {
throw new ArithmeticException("Second operand can't be zero in division");
}
stk.push(firstOperand/secondOperand);
break;
default:
break;
}
}
}
It's enough to write
for (String token : tokens) {
stk.push(processToken(stk, token));
}
and separately
public static String processToken(Stack stack, String token) throws ArithmeticException {
switch (token) {
case "+":
return stack.pop() + stack.pop();
case "-":
Integer subtrahend = stack.pop();
return stack.pop() - subtrahend;
case "*":
return stack.pop() * stack.pop();
case "/":
Integer divisor = stack.pop();
if (divisor == 0) {
throw new ArithmeticException("Divisor can't be zero");
}
return stack.pop() / divisor;
default:
return Integer.parseInt(token);
}
}
The advantage of this is that you only write each operator down once. So adding or removing an operator only needs to be done in one place. In the original code, you had to change in two places. This can lead to odd behavior if you make changes.
The separate method saves having to rewrite stk.push( constantly. Yes, we write return a lot, but we no longer need to break.
I would prefer the for each form or the advanced for loop to a C-style for loop anytime you don't use the index variable for anything other than indexing the collection.
You also might consider having a PostfixCalculator class with an object field of a Stack. Then you wouldn't have to pass the Stack around. Obviously the two methods would no longer be static in that case.
It's not clear to me that you gain anything by the check for division by zero. If you actually divide by zero, it will throw an ArithmeticException anyway. Why do so manually? Normally it would be because you wanted to add something to the error message or log something prior to throwing the exception. Or to keep going after handling the exception. But you don't do any of that here. So why the extra code? If there's a reason, put it in a comment so that no one undoes what you did.
It would make more sense to me to catch the exception for the stack being empty. That's likely to happen due to bad input. If you catch it, you can add additional information about the error. In particular, what token failed.
Similarly, you might want to handle the situation where the stack is not empty. If you have more than one value left on the stack after processing all the tokens, then there was a mistake somewhere.
Note that the JavaDoc for Stack says to use Deque instead.