Return to Answer

##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.

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.