current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite
1

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;
}
share|improve this question
    
We are not here to fix your code. And it is also very unclear what you mean by "adding the character = at the end". – Simon Forsberg♦ Nov 20 '13 at 12:54
up vote 2 down vote accepted

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.

share|improve this answer
    
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 – Max Nov 20 '13 at 13:03
    
@user1021726 Indeed possible, I was thinking that poppingResult1 and poppingResult2 would be more clear than having variations in the beginning like firstPoppingResult and secondPoppingResult‌​. – Simon Forsberg♦ Nov 20 '13 at 13:10
    
about the naming of my class, i was in a hurry and totally forgot about the convention :/ – Scarl Nov 20 '13 at 14:10
up vote 0 down vote

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.

share|improve this answer
    
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? – 200_success♦ Nov 8 '14 at 7:26
    
Thank you. This is indeed an alternative approach that may be worth considering in relation to originally submitted code. – Andrej Nov 10 '14 at 19:04
    
Dropping public and using shorter names is intentional. The goal is to make code shorter and easier to read. – Andrej Nov 10 '14 at 19:10

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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