2
\$\begingroup\$

I have written an infix-to-postfix program that I would like reviewed and with a few suggestions on improving it, such as a more modularized approach.

package basicstrut;

import java.util.Stack;

public class Infix {

    private String infixStr;

    private Stack<Character> oprStack;

    public Infix(String infixStr){
        oprStack = new Stack<Character>();
        this.infixStr = infixStr;
    }

    public String toPostFix(){
        StringBuffer buf = new StringBuffer(infixStr.length());

        for(int i=0;i<infixStr.length();i++){
            char str = (infixStr.charAt(i));            
            if(valueOfOpr(str) == 0){
                buf.append(str);

                if(i != infixStr.length()-1 && !oprStack.isEmpty()){
                    char nextChar = (infixStr.charAt(i+1));
                    char opStack = oprStack.peek();

                    int compare =  compareOperators (opStack,nextChar); 

                    if(compare >0 && valueOfOpr(opStack) != 5) {
                        opStack = oprStack.pop();
                        buf.append(opStack);
                    }
                }               
            }   

            if(i == infixStr.length()-1){
                if(valueOfOpr(str) > 0 && valueOfOpr(str) != 6) throw new RuntimeException("Invalid Expression");
                while(!oprStack.isEmpty()){
                    char temp = oprStack.pop();
                    if(valueOfOpr(temp) != 5)buf.append(temp);
                }
            }else if (valueOfOpr(str) > 0){

                if(oprStack.isEmpty()) {
                    oprStack.push(str);
                }else{ 

                    if(valueOfOpr(str) == 6){
                        while(!oprStack.isEmpty()){
                            char temp = oprStack.pop();
                            if(valueOfOpr(temp) != 5)buf.append(temp);
                        }                   
                    }else{
                        char opStack = oprStack.pop();
                        int compare =  compareOperators (opStack,str);  

                        if(compare >0) {
                            oprStack.push(str);oprStack.push(opStack);                  
                        }else{
                            oprStack.push(opStack);oprStack.push(str);
                        }   
                    }
                }                   
            }



        }

        return buf.toString();
    }


    public int compareOperators(char op1 , char op2){       
        return valueOfOpr( op1) - valueOfOpr( op2) ;
    }


    public int valueOfOpr(char op){
        int value = 0;

        switch (op) {
        case '*':
            value = 4;
            break;
        case '/':
            value = 3;
            break;
        case '+':
            value = 2;
            break;          
        case '-':
            value = 1;
            break;  
        case '[':           
        case ']':
        case '(':       
            value = 5;  
            break;
        case ')':
            value = 6;  
            break;          
        default:
            break;
        }

        return value;
    }



    public static void main(String[] args) {
        Infix i = new Infix("A+B");
/*      System.out.println("A+B -->"+i.toPostFix());
        i = new Infix("A+B-C");
        System.out.println("A+B-C -->"+i.toPostFix());

        i = new Infix("A+B*C");
        System.out.println("A+B*C -->"+i.toPostFix());
        i = new Infix("A*B+C");
        System.out.println("A*B+C -->"+ i.toPostFix());     
        i = new Infix("A/B+C");
        System.out.println("A/B+C -->"+i.toPostFix());
        i = new Infix("A+B/C");
        System.out.println("A+B/C -->"+ i.toPostFix());
        */
        i = new Infix("A*(B+C)");
        System.out.println("A*(B+C) -->"+ i.toPostFix());

        i = new Infix("A*(B+C)*(D+E)");
        System.out.println("A*(B+C)*(D+E) -->"+ i.toPostFix());

        i = new Infix("A*(B+C)-D/(E+F)");
        System.out.println("A*(B+C)-D/(E+F) -->"+ i.toPostFix());       

        i = new Infix("A*(B+C)-D/(E+F)*G+H");
        System.out.println("A*(B+C)-D/(E+F)*G+H -->"+ i.toPostFix());

        i = new Infix("(A*B*(C+D))");
        System.out.println("(A*B*(C+D)) -->"+ i.toPostFix());

        i = new Infix("(A*B*(C+D)*E+F)");
        System.out.println("(A*B*(C+D)*E+F) -->"+ i.toPostFix());

    }
}
\$\endgroup\$
4
  • \$\begingroup\$ @DaveJarvis What kind of life cycle for Infix are you imagining such that it ever needs to be thread-safe? \$\endgroup\$ Commented Jul 10, 2013 at 11:30
  • \$\begingroup\$ @abuzittingillifirca: Simultaneous calls to toPostFix will corrupt the oprStack variable. Since oprStack is only ever used by toPostFix, move it into that method. \$\endgroup\$ Commented Jul 10, 2013 at 20:45
  • \$\begingroup\$ @DaveJarvis You are absolutely right about oprStack belonging to toPostFix (Also infixStr should be final. ); with or without Infix needing to be thread-safe. And it needs not. Actually toPostFix will not be called in a thread twice either. Infix is just a function masquerading as a class. \$\endgroup\$ Commented Jul 11, 2013 at 6:06
  • \$\begingroup\$ @Dave - yes sure thing. \$\endgroup\$
    – Sudhakar
    Commented Jul 11, 2013 at 8:44

1 Answer 1

3
\$\begingroup\$

One small thing you can do is improve the readability of things like

valueOfOpr(opStack) != 5

by making an Operator enum like so (notice I took the liberty of re-ordering the square brackets, correct me if I'm wrong):

public static enum Operator {
    NOT_AN_OPERATOR(0, ""),
    SUBTRACTION(1, "-"),
    ADDITION(2, "+"),
    DIVISION(3, "/"),
    MULTIPLICATION(4, "*"),
    OPEN_BRACKET(5, "[("),
    CLOSE_BRACKET(6, "])");

    private final int precedence;
    private final String symbol;

    private Operator(int precedence, String symbol) {
        this.precedence = precedence;
        this.symbol = symbol;
    }

    public int getPrecedence() {
        return precedence;
    }

    public static boolean isOperator(char symbol, Operator operator) {
        return operator.symbol.indexOf(symbol) != -1;
    }
}

That way,

valueOfOpr(str) == 0
valueOfOpr(opStack) != 5

would become

!Operator.isOperator(str, Operator.NOT_AN_OPERATOR);
!Operator.isOperator(opStack, Operator.OPEN_BRACKET);

etc.

\$\endgroup\$
2
  • \$\begingroup\$ @kuporific: I'd recommend Operator.isOperator( str, Operator.NOT_AN_OPERATOR ) over using a get method. Typically get methods are not OO. \$\endgroup\$ Commented Jul 11, 2013 at 16:22
  • 1
    \$\begingroup\$ @DaveJarvis Thanks for the recommendation, I have edited the answer accordingly. isOperator produces a much cleaner solution (no more case/switch). \$\endgroup\$
    – kuporific
    Commented Jul 11, 2013 at 16:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.