Skip to main content
added 38 characters in body
Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

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.

Pointless constructor

This constructor is pointless, I suggest to remove it:

public Parser() {
    super();
}

Ignored return values

The returned values of convToPosfixQueue and collectParsedTokens are never used. I suggest to make these methods void.

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 in the result of the parsing, the output.

As such, this class is kind of ephemeral, its fields are used during the parsing process. Once the processing is completed, there's no more need for an instance, we're only interested in the output.

As such, it would be better to forbid instantiation, and provide a utility method instead:

private Parser() {
    throw new AssertionError("utility class, forbidden constructor");
}

public static Queue<String> toPosfixQueue(Queue<String> infixQueue) {
    Parser parser = new Parser();
    return parser.convToPosfixQueue(infixQueue);
}

With this change, the class has a more natural usage. You cannot create instances of it, but it provides a static utility method to perform the parsing by way of a private instance, hidden from users.

I would go one step further, and rename the class providing the utility method to something like ParserUtils, and move the current Parser class to a private static inner class.

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.

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.

Source Link
janos
  • 111.7k
  • 15
  • 152
  • 391

Pointless constructor

This constructor is pointless, I suggest to remove it:

public Parser() {
    super();
}

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.

Ignored return values

The returned values of convToPosfixQueue and collectParsedTokens are never used. I suggest to make these methods void.

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 in the result of the parsing, the output.

As such, this class is kind of ephemeral, its fields are used during the parsing process. Once the processing is completed, there's no more need for an instance, we're only interested in the output.

As such, it would be better to forbid instantiation, and provide a utility method instead:

private Parser() {
    throw new AssertionError("utility class, forbidden constructor");
}

public static Queue<String> toPosfixQueue(Queue<String> infixQueue) {
    Parser parser = new Parser();
    return parser.convToPosfixQueue(infixQueue);
}

With this change, the class has a more natural usage. You cannot create instances of it, but it provides a static utility method to perform the parsing by way of a private instance, hidden from users.

I would go one step further, and rename the class providing the utility method to something like ParserUtils, and move the current Parser class to a private static inner class.