3
\$\begingroup\$

This is a revised version of my previous code.

This application will take input string like "3 2 + 1 -" and output 4 because it is the reverse Polish notation for (3+2)-1 = 4. This solution focuses on the design of classes.The current code has 2 operators (plus and minus) in default. There are 2 constructors. 1 will create the default calculator. The other can take an additional HashMap that will map a string to an Operator.

The code works. Any further recommendations are much appreciated! Questions I have:

  1. Do I need to write getters for fields that I didn't use, such as public String getInput().
  2. I didn't write any setters for the non-static variables. Do I need them?

 import java.util.*;
 public class RPNCalculator {

    public Stack<Integer> stack;
    private String input;
    public Map<String, Operator> map;
    private String operators="";

    public RPNCalculator(String userInput){
        input = userInput;
        stack = new Stack<Integer>();
        // default hashmap initialization 
        map = new HashMap<String, Operator>();
        map.put("+", new Plus());
        map.put("-", new Minus());
        // this string is constructed for later use in processInput()
        for(String key: map.keySet()){
            operators+= key;
        }
    }
    public RPNCalculator(String userInput, HashMap<String, Operator> mapInput){
        input = userInput;
        stack = new Stack<Integer>();
        this.map = mapInput; 
        for(String key: map.keySet()){
            operators+= key;
        }
    }

    public Stack<Integer> getStack(){
        return this.stack;
    }
    public String getInput(){
        return this.input;
    }
    public void processInput(){
        StringTokenizer st = new StringTokenizer(input, " ");
        while (st.hasMoreTokens()){
            String str = st.nextToken();
            if(!operators.contains(str)){               //test whether the token is an operator 
                stack.push(Integer.parseInt(str));
            }
            else{
                map.get(str).calc(this);
            }
        }
        if(stack.size()!=1){
            System.out.println("The input expression is not the correct format");
        }else{
            System.out.println(stack.pop());
        }
    }
}

Other supporting classes and driver (contains main):

public abstract class Operator {
    abstract void calc(RPNCalculator rpn);
}
public class Minus extends Operator {
    @Override
    void calc(RPNCalculator rpn) {
        int a = rpn.getStack().pop();
        int b = rpn.getStack().pop();
        rpn.getStack().push(b-a);

    }
}
public class Plus extends Operator {
    @Override
    void calc(RPNCalculator rpn) {
        int a = rpn.getStack().pop();
        int b = rpn.getStack().pop();
        rpn.getStack().push(a+b);

    }
}
public class RPNDriver {
       public static void main(String[] args) {
           System.out.println("starting calculator...");
           RPNCalculator rpn = new RPNCalculator("1 21 + 2 -");
           rpn.processInput();
       }
}
\$\endgroup\$
2
\$\begingroup\$

Do I need to write getters for fields that I didn't use? such as public String getInput(). I didn't write any setters for the non-static variables. do I need them?

It depends. If you want to expose the field to the classes's clients, then you need a getter. For instance, it makes sense to write a getter for the stack in the calculator so that operators can use it (sure, you could have design it differently and pass a stack to an operator directly). It's really about the design of your class. There's no definitive answer. I don't think that you need a getter for the input here.

The operators String is redundant. You can check if a string is in the map using map's containsKey method.

I also think that the input should be passed as an argument to the processInput method, not to the constructor. I just makes more sense to me to build one calculator for a specific sets of operators and then run it on as many different inputs as you need. I don't think that the input is the essential part of the calculator entity. It's more like something the calculator can processes, not something it consists of.

I'd also recommend to decouple the computations from the output. Each method should do one focused thing. The processInput should do exactly this: process the input and return the result (or throw an exception). Printing it is a different concern. It also makes your code more reusable: if you just return the value, you can still use the class if just need to evaluate the expression without printing it or if your need to display it differently.

The comments should not repeat the code so something like is // default hashmap initialization is even worse than no comments as it clutters the code. It just creates more noise. It's clear that a new HashMap<>(); creates a new empty map. They should rather explain why specific decisions were made. It's also a good practice to write doc comments for your classes and methods to specify it's behavior (including what kind of exceptions it throws) so that one doesn't need to read the code of the method to figure out what it does and how it (and shouldn't) be used.

I'd also suggest to create a copy of the mapInput in this public RPNCalculator(String userInput, HashMap<String, Operator> mapInput) constructor (so that the map used by the calculator doesn't change even if the mapInput is later changed by the client). Either way, you need to document this kind of decisions so that the users of your class know if it's safe to change the map passed to the constructor later in their code.

\$\endgroup\$
  • \$\begingroup\$ thanks! I changed processInput() to return int. And you pointed out that my constructor takes an input string, which it really shouldn't. And that's really important. I really learned a lot from your answer. Before I construct an object, I need to think about "what I want to use this object for" vs "what this object is made of" \$\endgroup\$ – Angela Pan May 24 '17 at 15:17
2
\$\begingroup\$

Constructors

  • If you have two constructors and have to explain its functionality with a sentence each, it's usually a good idea, to create a static factory and give the create-methods a proper name.
  • You have code duplication in your constructors. In the upper one, you only need to create the map and call the other constructor using this(userInput, map)
  • The second constructors gets a HashMap, should be a Map ("always develop against interfaces")

naming

Always try to be as specific as possible. For instance stack and map: I actually have to read code to understand, what you want to put into them. So maybe call the map signToOperationMap, and the stack maybe numbersToCalculate

abstraction

I don't see any need for an abstract class, an interface Operator should be enough.

circular dependencies

The Operator.calc method actually takes an RPNCalculator parameter, at the same time RPNCalculator has a dependency to the Operator type. This is called a circular dependency and must be avoided. One reason why it's bad, is the tight coupling of two components. So you can not reuse your Operator classes without RPNCalculator.

You either pass the values which have to be calculated, return the result and push them back within RPNCalculator, or, your Operator "Layer" provides an interface with pop and push methods, which then will be implemented by the RPNCalculator which you then will pass. So in any other calculator, your operators can be reused.

your question

Do I need to write getters for fields that I didn't use? such as public String getInput(). I didn't write any setters for the non-static variables. do I need them? In most of the time, you shouldn't do that, because it's dead code. Generated code is the exception. Maybe there's others, but I can't think of any.

Hope this helps.

\$\endgroup\$
  • \$\begingroup\$ "I don't see any need for an abstract class, an interface Operator should be enough." can you explain more why that is? Thanks. \$\endgroup\$ – Angela Pan May 23 '17 at 14:37
  • \$\begingroup\$ Because with the actual code, an interface does the job. In general, I'd say, you need a reason, to write an abstract class, and not the other way around (need a reason not to), since it can come with restrictions. \$\endgroup\$ – slowy May 24 '17 at 11:13
1
\$\begingroup\$

Personally, I'd replace the Map map, and the operators String with an enum, something like:

public enum Operation {
  PLUS("+", new Plus()),
  MINUS("-", new Minus());

  private String sign;
  private Operator operator;

  private Operation(String sign, Operator operator) {
    this.sign = sign;
    this.operator = operator;
  }

  public String getSign() {
    return sign;
  }

  public Operation parseSign(String sign) {
    for (Operation operation : Operation.values()) {
        if (operation.getSign().equals(sign)) {
            return operation;
        }
    }

    return null;
  }
}

Then your if statement in processInput becomes something like:

Operation operation = Operation.parseSign(str);
if (operation == null) {
  stack.push(Integer.parseInt(str));
} else {
  operation.calc(this);
}

This encapsulates the operations in a single place, and means you only have one class (the enum) to change when you want to add, modify or remove operations. Although this is a very small benefit at the moment, compared to your current implementation, as your application grows, and becomes more complex, grouping related functionality together and separating it from unrelated functionality will help make it easy to maintain. For a better explanation of this, have a look into the Single Responsibility Principle, as well as the principles of high cohesion and loose coupling.

There's obvious scope for further improving this - if you don't want to iterate over each of the values of Operation each time you check a sign (I'm not sure I like the name sign for that variable, but couldn't think of anything better), you could use some different data structure behind the scenes inside Operation.

Also, because Operator.calc operates on the RPNCalculator class itself, you could make pushing a number onto the stack an operation in itself (i.e. an implementation of Operator), and instead of parseSign returning null if it doesn't match one of the characters it's expecting, have it return that operation, which removes the need for the if statement in processInput altogether, so, we'd add:

PUSH_INTEGER(null, new PushInteger());

and instead of return null in parseSign(), we'd have:

return PUSH_INTEGER;

The calc method of PushInteger would then just push the given value onto the stack. If you do this, calc probably isn't the best name for that method.

\$\endgroup\$
  • 1
    \$\begingroup\$ thanks a lot for the feedbacks, I have a follow-up question. You mentioned "you only have one class (the enum) to change when you want to add more operations" My current code only needs to add a new mapping value if I need to add more operations too. What are the benefits of using the Enum class? (I'm not familiar with the Enum usage) \$\endgroup\$ – Angela Pan May 23 '17 at 14:43
  • \$\begingroup\$ Good point, I didn't explain that well; I've added some extra information to my answer to hopefully do a better job. \$\endgroup\$ – DaveyDaveDave May 23 '17 at 15:02

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.