I am doing a practice project to create a calculator with OOP principles. Below is my CalculatorApp that communicates with Calculator(to calc integers) and StringParser(to tokenize input). For CalculatorApp to work, strict rules need to follow. For example, the input must use “add” not “plus” and “(” not any other kind. Therefore I have so many hardcoded variables in the class. Is it a bad design? Am I doing too much in process and is creating variables (i.e wordToSign_map) on the fly bad practice? Should I refactor all the variables to another class? This is for a job so I need to make sure I do my best. Any feedback will be greatly appreciated.

public class CalculatorApp {

    String input;
    Calculator calculator;
    int result;

    final String open_parentheses = "(";
    final String close_parentheses = ")";
    final String delimiter = " ";


    public CalculatorApp(Calculator calculator){
        this.calculator = calculator;
    }

    public CalculatorApp input (String input){
        this.input = input;
        return this;
    }

    public CalculatorApp process (){
        if(!isExpression()){
            result=Integer.parseInt(input);
            return this;
        }

        Map<String, String> wordToSign_map = new HashMap<>();
        wordToSign_map.put("add", "+");
        wordToSign_map.put("multiply", "*");

        Map<String, String> parentheses_spacing_map = new HashMap<>();
        parentheses_spacing_map.put(open_parentheses, open_parentheses+delimiter);
        parentheses_spacing_map.put(close_parentheses, delimiter+close_parentheses);

        StringParser stringParser = new StringParser(input, wordToSign_map, parentheses_spacing_map, delimiter);

        String[] parsedString = stringParser.TokenizeExpression();

        Set<String> operatorSigns = new HashSet<>();
        operatorSigns.addAll(wordToSign_map.values());

        try {
            Tree tree = new Tree();
            tree.constructATree(parsedString, operatorSigns, open_parentheses, close_parentheses);
            result = evaluateExpressionTree(tree.getRoot());
        } catch (EmptyStackException e) {
            System.out.println("Invalid input");
            throw e;
        }
        return this;
    }

    public int output (){
        return result;
    }

    private boolean isExpression(){
        return input.contains(open_parentheses) && input.contains(close_parentheses);
    }
}

This is my Tree class. It takes a string list and creates nodes and children around the brackets and +,* signs. That’s why the string has to be exactly one-spaced and can’t contain any unknown signs. Should I leave the comments there? Is it too messy and hard to read? Is passing too many args in the method call bad, should I instead pass them in the constructor and hold them as fields?

    public class Tree {

    Node root;

    public Node getRoot() {
        return root;
    }

    public void constructATree (String[] parsedString, Set<String> operatorSigns, String startExp, String endExp) throws EmptyStackException {
        Stack<Node> localRoot = new Stack<>();
        for (int i = 0; i < parsedString.length; i++) {
            if(parsedString[i].equals(startExp)) { // ignore (
                continue;
            }
            if(operatorSigns.contains(parsedString[i])){ // operator for expression
                localRoot.push(new Node(parsedString[i], true));

            } else if(parsedString[i].equals(endExp)) // end of an expression
            {
                Node tempNode = localRoot.pop(); // get the last node
                if(localRoot.empty()){
                    this.root = tempNode; // if there is only one expression return it as the root of the tree
                } else {
                    Node tail = localRoot.peek(); //
                    tail.addChild(tempNode); // add the children from last node to trailing node
                }
            } else {
                Node tempNode = localRoot.pop();
                tempNode.addChild(new Node(parsedString[i], false)); // add values to children of operator node
                localRoot.push(tempNode);
            }
        }
    }
}

app.input(input);
app.process();
var result = app.output();

This is a bad practice. Every function/method has means of passing input, it has body and it has return value. Setting the input to class field, then calling method without arguments to process the set input, then storing result to another class field, then calling another method to retrive the result is utterly complicated, confusing and error prone.

You can forget to call input and call process directly which will fail or process the previous input.

You can forget to call process and retrieve an unexpected null result or retrieve the result of previous process.

You can have momory occupied longer then necesary even if you don’t care for the result anymore.

Why not use all the channels of a function?

var result = app.process(input);

Now imagine the result has been printed for example and it is no longer needed. In your implementation the memory is still siezed (because app object holds reference to it) until the app object is destroyed. In my implementation the momory is released (once the scope of the variable is left) regardless of the app object being destroyed or not.

Returning this

Be very thouthful when returning this. You hardly ever need to return this. It only leads to thoughts about saving outputs to the class fields, because it feels like return value is already siezed by this and thus the real result must be provided through a different channel. But true is that this is always available to the caller (because they know the reference to the object) and so if the return value channel can be used for something more meaningful, you should do it. And even if there is nothing meaningful to return, you should consider returning void, instead of this.

Comments

if your code needs a lot of comments to be understandable, it probably lacks some structure (split to more methods maybe…). But maybe some of the comments are not necesary, like here:

Node tempNode = localRoot.pop(); // get the last node

I dont think that comment tells me anything I couldn’t infer from the code itself…

Passing too many arguments

That’s disputable if it’s bad to pass a lot of arguments. If they are needed, then they have to be there. If they repeat on more places a simple data structure could be introduced to condense them together.

Passing them as constructor arguments versus passing them as method arguments is not a good question. They have different purpose. The method should accept all arguments that are available to the caller. Constructor should accept all arguments that are only known to the creator of the object.

If some arguments known to the caller are to be passed through constructor, the caller of the method must also be the creator of the object and that’s often not true. In fact, if you maintain single responsibility of all classes, it will never be true.

Class fileds are like configuration of the object – how it should behave, what it can offer. Method arguments are more like a specification of what the caller wants.