2
\$\begingroup\$

I just wrote the code to evaluate a postfix expression and would like it if someone reviews it for me, it works perfectly fine but I'm having trouble adding the character "=" at the end.

 public static int evalPostfix(String exp) {

    int res = 0;

    myStack list = new myStack();
    int n1;     //result of 1st popping
    int n2;     // result of 2nd popping


    for (int i = 0; i < exp.length(); i++) {
        char ch = exp.charAt(i);


            if (ch == ' ') {
            } else {
                if (ch > '0' && ch < '9') {
                    list.push(ch);
                    //          list.printS();
                } else {
                    n1 = Integer.parseInt("" + list.pop());
                    n2 = Integer.parseInt("" + list.pop());

                    switch (ch) {
                        case '+':
                            list.push(n1 + n2);
                            break;
                        case '-':
                            list.push(n1 - n2);
                            break;
                        case '*':
                            list.push(n1 * n2);
                            break;
                        case '/':
                            list.push(n1 / n2);
                            break;

                        default:
                            System.out.println("Invalid operator order!");
                    }
                }
            }
        }

    res = Integer.parseInt("" + list.pop());

    return res;
}
\$\endgroup\$
2

2 Answers 2

2
\$\begingroup\$

Conventions

According to coding conventions, Java class names should start with an uppercase letter. myStack should be MyStack.

Variable names

Whenever you have a comment after declaring a variable, rename the variable to comment itself.

int n1;     //result of 1st popping

Can be renamed to make the variable name "comment itself".

int poppingResult1;

If this, then do nothing, else do something

if (ch == ' ') {
} else {

To achieve the same effect, and make the code cleaner, write instead:

if (ch != ' ') {

Integer.parseInt

Now, it's not clear which datatype you get from list.pop(). If it is an int (or Integer), then variable = list.pop() will be sufficient.

If it is an Object, instead use Integer.parseInt(String.valueOf(list.pop()))

Anyway, if possible, change the values you store in your list to always be of the same type (Integer, preferably). This would make your code cleaner as you don't have to use Integer.parseInt every time you pop something of the list.

\$\endgroup\$
3
  • \$\begingroup\$ May I suggest that instead of 'int poppingResult1' he should name it 'int firstPoppingResult' as his comment suggests that it's the result of 1st popping \$\endgroup\$
    – Max
    Nov 20, 2013 at 13:03
  • \$\begingroup\$ @user1021726 Indeed possible, I was thinking that poppingResult1 and poppingResult2 would be more clear than having variations in the beginning like firstPoppingResult and secondPoppingResult. \$\endgroup\$ Nov 20, 2013 at 13:10
  • \$\begingroup\$ about the naming of my class, i was in a hurry and totally forgot about the convention :/ \$\endgroup\$
    – Scarl
    Nov 20, 2013 at 14:10
0
\$\begingroup\$

Consider using approach below where input string contains valid postfix expression.

static int evalPf(String str)
{
    Scanner sc = new Scanner(str);
    Stack<Integer> stack = new Stack<Integer>();

    while (sc.hasNext()) {
        if (sc.hasNextInt()) {
            stack.push(sc.nextInt());
            continue;
        }
        int b = stack.pop();
        int a = stack.pop();
        char op = sc.next().charAt(0);
        if      (op == '+') stack.push(a + b);
        else if (op == '-') stack.push(a - b);
        else if (op == '*') stack.push(a * b);
        else if (op == '/') stack.push(a / b);
        else if (op == '%') stack.push(a % b);
    }

    sc.close();
    return stack.pop();
}

In addition to simplifying the logic, I've dropped the public access modifier and used shorter names, with the goal of making the code shorter and easier to read.

\$\endgroup\$
4
  • \$\begingroup\$ Welcome to Code Review! While you have presented a very nice looking solution, your answer doesn't actually review the code. Your implementation contains quite a lot of differences from the original (dropping public; renaming to evalPf, op, a, and b; adding a modulo operator). Are all of your changes intentional, and could you discuss why you made them? \$\endgroup\$ Nov 8, 2014 at 7:26
  • \$\begingroup\$ Thank you. This is indeed an alternative approach that may be worth considering in relation to originally submitted code. \$\endgroup\$
    – Andrej
    Nov 10, 2014 at 19:04
  • \$\begingroup\$ Dropping public and using shorter names is intentional. The goal is to make code shorter and easier to read. \$\endgroup\$
    – Andrej
    Nov 10, 2014 at 19:10
  • \$\begingroup\$ Then I would say the evalPf is less clear than evalPostfix. You could also use a switch on op` instead of if .. else if \$\endgroup\$
    – gervais.b
    Aug 18, 2016 at 10:03

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