3
\$\begingroup\$

This solution focuses on the design of classes, not the actual push and pop stack part. I will include that part of the code as well. The current code has 2 operators (plus and minus).

If we add another subclass under Token, is there a way that we don't need to check in processInput() what each operator is and perform the right calculation?

I didn't know how to answer that question. I looked into Java Reflection, but still don't know how reflection can help me in this case. Can anyone shed some light on how I can make this design better?

I want to allow people to add new operators, multiply and divide, and maybe make their own definition for special operators.

This application will take input string like "1 2 + 1 -" and output 2 because "1 2 + 1 -" is the reverse Polish notation for (1+2)-1 = 2.

import java.util.*;
public class RPNCalculator {
    public static Stack<Integer> stack;
    //assume this string has integers and legal operators +,- and deliminated by space
    // an example would be "12 2 + 1 -" 
    private static String input;
    static String operators ="+-";

    public RPNCalculator(String userInput){
        input = userInput;
        stack = new Stack<Integer>();
    }

    public void processInput(){
        StringTokenizer st = new StringTokenizer(input, " ");
        while (st.hasMoreTokens()){
            String str = st.nextToken();
            if(!operators.contains(str)){
                stack.push(Integer.parseInt(str));
            }
            else{
                if(str.equals("+")){
                    Plus obj = new Plus();
                    obj.calc();
                }
                else if(str.equals("-")){
                    Minus obj2 = new Minus();
                    obj2.calc();
                }
            }
        }
        System.out.println(stack.pop());
    }
}

public abstract class Token {
    abstract void calc();
}
public class Minus extends Token {

    @Override
    void calc() {
        int a = RPNCalculator.stack.pop();
        int b = RPNCalculator.stack.pop();
        RPNCalculator.stack.push(b-a);

    }

}

public class Plus extends Token {

    @Override
    void calc() {
        int a = RPNCalculator.stack.pop();
        int b = RPNCalculator.stack.pop();
        RPNCalculator.stack.push(a+b);
    }

}
public class RPNDriver {
       public static void main(String[] args) {
           System.out.println("starting calculator...");
           RPNCalculator rpn = new RPNCalculator("1 2 +");
           rpn.processInput();
       }
}
\$\endgroup\$
4
  • \$\begingroup\$ [cry] why did I get down voted? I wrote these 5 classes. and I know it's not the ideal object oriented design. some feedback is much appreciated. \$\endgroup\$ Commented May 19, 2017 at 20:17
  • \$\begingroup\$ I have tested the code. All runs correctly. see example tested in the Driver class. I'm looking for ways where I can improve the structure for adding more operators in the future. \$\endgroup\$ Commented May 19, 2017 at 20:22
  • \$\begingroup\$ I am not the person who downvoted you. There is also a vote-to-close as Unclear What You're Asking. That means that someone wanted more explanation of how the code works. E.g. what input does it take? How does it process the input? What output does it produce? How is operator extension supposed to work? If you add that information to the question, it's possible that the downvote and vote-to-close may be retracted. \$\endgroup\$ Commented May 19, 2017 at 20:57
  • \$\begingroup\$ sorry if I made the question confusing. If I make another subclass, for example multiply extends token. I also need to go into the processInput() to update the logic in there. Is there a way where I don't update the logic in processInput()?? Because the idea of using OO design is to module things. If I'm updating the logic every time a operator is added, why don't I just do If (str =='+' ){ a+b }; \$\endgroup\$ Commented May 19, 2017 at 21:15

1 Answer 1

Reset to default
2
\$\begingroup\$

You can add more flexibility by storing a mapping from a String to a Token in a Map. After populating the map, you'll just need to look up for the token in it instead of writing a a long sequence of if-else blocks.

That is, you can define and populate it like this:

Map<String, Token> operators = new HashMap<>();
operators.add("+", new Plus());
// other operators go here

and then use it like this: operators.get(str).calc()

This way, you just need to define a new subclass of the Token and add its instance to the operators map to define a new operator.

You can make the code even more flexible by passing this map as a parameter to the processInput method to decouple reading the input from the computations (this way, a client will be able to define his own subclass of the Token and pass it to this method).

I'd also rename the Token. Operator or Operation seems more appropriate to me (because a number is also a token, so this name is more precise).

There's also no point in having static members in the RPNCalculator class. It makes it non-reusable and might create a bunch of issues in a multithreaded environment. I'd suggest to make all members non-static and encapsulate them properly (and pass a reference to an instance of this class to the constructor of the concrete Token subclasses).

You can also decouple the operators from the calculator completely by changing the signature of the calc method to public int calc(int a, int b) (it might be an issue for non-binary operators, though).

You can improve your code by handling the errors more carefully. For instance, if the number of tokens on the stack is more than one in the end of the evaluation, your code will return some value, but it should be an error ("1 2" is not a valid expression, is it?). You can check such cases and throw an appropriate exception.

\$\endgroup\$
4
  • \$\begingroup\$ thank you! I think HashMap is what I'm looking for! a follow-up question, when you say make all members non-static, you mean the static String input, stack and string operators, right?? (I'm new to OO design). any suggestions are much appreciated! \$\endgroup\$ Commented May 19, 2017 at 21:28
  • \$\begingroup\$ Definitely don't change it to public int calc(int a, int b). RPN operators should manipulate the stack directly. \$\endgroup\$ Commented May 19, 2017 at 21:33
  • \$\begingroup\$ @AngelaPan Yes, it refers to the input String and the stack. \$\endgroup\$ Commented May 19, 2017 at 21:46
  • \$\begingroup\$ 200_success, I read your solution in the link where there is a static hashmap in the operator class, is this for decouple the calculator class from the operators? \$\endgroup\$ Commented May 19, 2017 at 21:48

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.