Infix to Postfix Converter Program

This is my homework. Kindly help me check it?

The instructions are:

Implement an infix expression to postfix expression converter. You are to implement the infix to postfix algorithm presented in the lecture. You are to use only the stack that was provided in the lab lecture. The use of the stack of JDK or any other stack is not allowed. The supported operators are +, -, *, / and ^. Grouping operator are ( and ).

Input: Infix expresseion where each token (operand and operator) are space-separated (console-based). Explore the StringTokenizer class for tokenizing (separating the infix expression into tokesn) the input. The book of Deitel and Deitel has examples on how to use StringTokenizer.

Output: Postfix expression (console-based). The tokens (operand and operator) are also space-separated in the output. "

The code I wrote is:

```
import java.util.*;

public class PostfixConverter {
private String infixExpression;

public PostfixConverter(String infixExpression) {
this.infixExpression = infixExpression;
}

private String convertInfixExpression() {
StringTokenizer tokens = new StringTokenizer(infixExpression);
Stack operatorStack = new ArrayStack();
String converted = "";

while(tokens.hasMoreTokens()) {
String token = tokens.nextToken();
if(token.equals("(")) {
operatorStack.push(new String(token));
}
else if(token.equals(")")) {
while(operatorStack.top().equals("(") != true) {
converted = converted + " " + operatorStack.pop();
}
if(operatorStack.top().equals("(")) {
operatorStack.pop();
}
}
else if (isOperator(token)){
if(operatorStack.isEmpty() == true) {
operatorStack.push(new String(token));
}
else {
if(ICP(token) < ISP((String) operatorStack.top())) {
converted = converted + " " + operatorStack.pop();
operatorStack.push(token);
}
else {
oper

I like this opportunity for a code review because I hopefully can share some good programming techniques that I didn't have a chance to learn until I actually got out of school and into the industry. I can edit to explain anything further.

-
Your variable infixExpression can be defined as final:

private final String infixExpression;

This isn't necessary but is good practice as it's a good idea to maximize the immutability of your classes. Anything variable which is only modified when declared, or in a constructor, may be final.

-
The convertInfixExpression method should also be public. I know this is for an assignment, but that should not stop you from thinking about using it outside the main method. Alternatively, you could do away with the constructor altogether, and just make this method static. That way, you would use it by calling

PostfixConverter.convertInfixExpression("my infix expression");

This is possible by changing your current convertInfixExpression method signature to

public static String convertInfixExpression(String infixExpression)

-
I don't know what library ArrayStack came from, but I can assume how it works. That being said, you don't parametize it which is a big no no, and causes yourself some extra work later. This should be

Stack operatorStack = new ArrayStack();

-
In Java, appending to a String is typically a bad idea. If you have an object you know you'll be appending too, use a StringBuilder:

StringBuilder converted = new StringBuilder();

-
Concerning the first while loop in the method, I don't like the looks of it from a glance. There's way too many nested while, if, and else statements. We'll see if we can simplify some of that. To start, let's look at the highest level of what's going on:

If token is a '(' push to stack
If token is a ')' do complicated stuff
If token is an Operator do complicated stuff
Otherwise append the token to the result

So the hint here is that you want to move all that complicated stuff to separate methods. Your code benefits from this by being much easier to read and reason about.

-
There's almost never a reason to use the new keyword next to String in Java.

operatorStack.push(token);

not

operatorStack.push(new String(token));

-
The first complicated block (fixed formatting):

while(operatorStack.top().equals("(") != true) {
    converted = converted + " " + operatorStack.pop();
}
if(operatorStack.top().equals("(")) {
    operatorStack.pop();
}

I guess this isn't too complicated, but I don't like seeing nested while statements... This is how I would write it:

while(!operatorStack.top().equals("(")) {
    converted.append(" ").append(operatorStack.pop());
}
operatorStack.pop();

I'll highlight the differences:

• There's no reason to ever compare anything to true or false in an if statement.

• Used append from the StringBuilder. Check out the Oracle tutorial on this class to find out why.

• There's no reason for the if statement. We already know the a ( is on the top of the stack, otherwise the previous loop wouldn't have ended.

-
Regarding the second complicated bit, I would write it like so:

else if (isOperator(token)){
    if(operatorStack.isEmpty()) {
        operatorStack.push(token);
    }
    else {
        if(ICP(token) < ISP(operatorStack.top())) {
            converted.append(" ").append(operatorStack.pop());
            operatorStack.push(token);
        }
        else {
            operatorStack.push(token);
        }
    }
}

Pretty much the same comments as above, but notice you don't need to cast to a String because you've now parametized your stack. Too be honest, though I'm still not entirely sure of the purpose of the logic here, but mainly because I have no idea what ICP and ISP mean!

-
Use descriptive and conventional method names. If ICP and ISP return a value, they should start with get. I know they're return the precedence of operators, but the S and C is still unclear to me. That being said, you can simplify all of this by using some Map constants. For example,

private static final Map S_PRECEDENCE;
static {
    Map map = new HashMap();
    map.put("+", 2);
    map.put("-", 2);
    map.put("*", 4);
    ....
    S_PRECEDENCE = Collections.unmodifiableMap(map);
}

And then you can change the ISP method to something like

public int ISP(String token) {
    return S_PRECEDENCE.contains(token) ? S_PRECEDENCE.get(token) : 0;
}

I'll note that S_PRECEDENCE is a bad name and should be expanded with whatever S means.

-
Along the same lines, you can make a Set of your operators, and simply check if that collection contains the token.

StackExchange Code Review Q#59565, answer score: 7