Infix to postfix parser in java

Question

Please review my Parser class. Should I declare map outside of convToPosfixQueue method?

class Parser {
 private final Stack<String> tempStack = new Stack<>();
 private final Queue<String> postfixQueue = new LinkedList<>();
 public Parser(Queue<String> infixQueue) {
 convToPosfixQueue(infixQueue);
 }
 public Parser() {
 super();
 }
 interface ProcessingMethod {
 void method(String token);
 }
 public Queue<String> convToPosfixQueue(Queue<String> infixQueue) {
 Map<String, ProcessingMethod> methodMap = new HashMap<>();
 methodMap.put("(", this::parseOpenBracket);
 methodMap.put(")", this::parseCloseBracket);
 methodMap.put("+", this::parseOperator);
 methodMap.put("-", this::parseOperator);
 methodMap.put("*", this::parseOperator);
 methodMap.put("/", this::parseOperator);
 infixQueue.stream().forEach(token -> {
 if (!isNumber(token)) {
 methodMap.get(token).method(token);
 } else
 parseNumber(token);
 });
 return collectParsedTokens();
 }
 private void parseOpenBracket(String token) {
 tempStack.push(token);
 }
 private void parseCloseBracket(String token) {
 while (!tempStack.empty()
 && !isOpenBracket(tempStack.lastElement())) {
 postfixQueue.add(tempStack.pop());
 }
 tempStack.pop();
 }
 private void parseNumber(String token) {
 postfixQueue.add(token);
 }
 private void parseOperator(String token) {
 while (!tempStack.empty()
 && isOperator(tempStack.lastElement())
 && getPrecedence(token) <= getPrecedence(tempStack.lastElement())) {
 postfixQueue.add(tempStack.pop());
 }
 tempStack.push(token);
 }
 private Queue<String> collectParsedTokens() {
 while (!tempStack.empty()) {
 postfixQueue.add(tempStack.pop());
 }
 return postfixQueue;
 }
 private boolean isNumber(String token) {
 return token.matches("[-+]?\\d*\\.?\\d+");
 }
 private boolean isOpenBracket(String token) {
 return token.equals("(");
 }
 private boolean isCloseBracket(String token) {
 return token.equals(")");
 }
 private boolean isOperator(String token) {
 String OPERATORS = "+-*/";
 return OPERATORS.contains(token);
 }
 private byte getPrecedence(String token) {
 if (token.equals("+") || token.equals("-")) {
 return 1;
 }
 return 2;
 }
}

• Simon Forsberg: Welcome to Code Review. Unfortunately I am at this moment down-voting this question because I personally do not consider it a good question. With a few changes to your question, you can make it much more clearer and more interesting for reviewers. I am hoping to give you an upvote soon.

Answer 1

Pointless call to super()

Calling super() when the superclass is Object is pointless:

public Parser() {
 super();
}

You can safely remove that call.

Don't do work in the constructor

It's considered bad practice to do work in a constructor, as in this one:

public Parser(Queue<String> infixQueue) {
 convToPosfixQueue(infixQueue);
}

Although you extracted the work to another method, that doesn't change the fact that this class does all its work at construction time.

I suggest to remove this constructor, and let users start the parsing by explicitly calling the convToPosfixQueue method.

With this constructor removed, you can remove the other one too, as the empty parameterless constructor will be given by the compiler for free.

Hide implementation details

As the ProcessingMethod interface is implementation detail, it would be good to make it private:

private interface ProcessingMethod {
 void method(String token);
}

On closer look, I'm not sure it's really all that useful. Its only purpose seems to be to serve as a switch when processing tokens. I suggest to use a simple switch and let the compiler optimize that to a map if it wants to.

Using constants

OPERATORS inside isOperator is a constant, so it would be better to move that to a private static final variable.

Optimization for methodMap

You create a new methodMap every time convToPosfixQueue is called. As the content of this map is always the same, it can be field, and initialized once at construction time:

private final Map<String, ProcessingMethod> methodMap = new HashMap<>();
{
 methodMap.put("(", this::parseOpenBracket);
 methodMap.put(")", this::parseCloseBracket);
 methodMap.put("+", this::parseOperator);
 methodMap.put("-", this::parseOperator);
 methodMap.put("*", this::parseOperator);
 methodMap.put("/", this::parseOperator);
}

Bug?

This class can be used only once, because after a call to convToPosfixQueue with non-empty input, tempStack and postfixQueue are not cleared. So when calling the method again, the parsed tokens from the previous call will still be there.

This can be fixed by clearing tempStack and postfixQueue as the first action of convToPosfixQueue.

Thread safety

The class is not thread-safe, as concurrent calls to convToPosfixQueue will manipulate the shared internal state. Perhaps thread-safety is not one of your design goals, but it's good to document this in JavaDoc.

Answer 2

First of all: It's never a bad idea to explain what you want your code to do since it does help everyone involved. It often helps understanding a certain problem better by explaining it to other people, and the community on here will have a much easier time reviewing your code when knowing what your goal is.

Regarding your question: Reading the documentation about the tools that you're using can benefit you a lot, in this case Maps. In the documentation it says that the left row of the Map contains the key, which can not have any duplicates. Since you are using String literals as keys your map will always be completely overridden once your convToPosfixQueue() method gets called, thus making the generation of this map for every method call unnecessary.

It won't be that relevant if you're aiming to maximize your performance, but it is good practice.