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\$
  • \$\begingroup\$ @DaveJarvis What kind of life cycle for Infix are you imagining such that it ever needs to be thread-safe? \$\endgroup\$ – abuzittin gillifirca Jul 10 '13 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\$ – Dave Jarvis Jul 10 '13 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\$ – abuzittin gillifirca Jul 11 '13 at 6:06
  • \$\begingroup\$ @Dave - yes sure thing. \$\endgroup\$ – Sudhakar Jul 11 '13 at 8:44
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\$
  • \$\begingroup\$ @kuporific: I'd recommend Operator.isOperator( str, Operator.NOT_AN_OPERATOR ) over using a get method. Typically get methods are not OO. \$\endgroup\$ – Dave Jarvis Jul 11 '13 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 Jul 11 '13 at 16:52

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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