Pointless constructorcall to super()
This constructorCalling super()
when the superclass is Object
is pointless, I suggest to remove it:
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.
Ignored return valuesOptimization for methodMap
The returned values of convToPosfixQueue
andYou create a new collectParsedTokensmethodMap
are never used. I suggest to make these methodsevery time voidconvToPosfixQueue
.
Don't do work in the constructor
It's considered a bad practice to do work in a constructor. Although you extracted the work to another method, that doesn't change much the fact that this class does all its work at construction time.
As it stands, the usage of this class is a bit strange. In a normal usage you create an instance and then do something with it. Often pass it around inside the program. When seeing an instance of a class called "Parser", I would think I can parse something with that object. But that's not the case here. By the time the instance is created it has already done its job, it's not intended to parse anything anymore. We're just interested inAs the resultcontent of the parsing, the output.
As such, this class is kind of ephemeral, its fields are used during the parsing process. Once the processingmap is completed, there's no more need for an instance, we're only interested inalways the output.
As suchsame, it wouldcan be better to forbid instantiationfield, and provide a utility method insteadinitialized once at construction time:
private Parserfinal Map<String, ProcessingMethod> methodMap = new HashMap<>() ;
{
throwmethodMap.put("(", newthis::parseOpenBracket);
AssertionError("utility class, forbidden constructor"methodMap.put(")", this::parseCloseBracket);
}
public static Queue<String> toPosfixQueue methodMap.put(Queue<String>"+", infixQueuethis::parseOperator) {;
ParsermethodMap.put("-", parserthis::parseOperator);
= new Parser methodMap.put("*", this::parseOperator);
return parsermethodMap.convToPosfixQueueput(infixQueue"/", this::parseOperator);
}
Bug?
With this change, theThis class has a more natural usage.
You cannot create instances of itcan be used only once,
but it provides because after a static utility methodcall to performconvToPosfixQueue
with non-empty input, tempStack
and postfixQueue
are not cleared. So when calling the parsing by way of a private instancemethod again, hiddenthe parsed tokens from usersthe previous call will still be there.
I would go one step further, and rename the class providing the utility method to something likeThis can be fixed by clearing ParserUtilstempStack
, and movepostfixQueue
as the currentfirst action of ParserconvToPosfixQueue
.
Thread safety
The class is not thread-safe, as concurrent calls to a private staticconvToPosfixQueue
inner classwill manipulate the shared internal state. Perhaps thread-safety is not one of your design goals, but it's good to document this in JavaDoc.