The following program implements a calculator that takes string inputs in the form of two values separated by an operator. It splits the inputs in to their parts, and then applies the operation, and outputs the result.

As per my understanding I implemented it using Object Oriented patterns.

I am a beginner to Java Programming and OOPs. It would be great help if somebody review the code and provide comments.

SimpleCalculator

package com.main;



import com.calculator.Calculator;

import com.calculator.Division;

import com.factory.CalculatorFactory;

import com.validator.NumberValidator;

import com.validator.OperatorValidator;

import com.validator.Validator;

import com.validator.ZeroValueValidator;



public class Main {

    public static void main(String args) {



        Validator numberValidator = new NumberValidator();

        Validator zeroValidator = new ZeroValueValidator();

        Validator operatorValidator = new OperatorValidator();



        String inputDatas = {"1234 + 23.45","3444 - 445","97 * 8","999 / 99.2","1234a + 23.45","3444  f445","97 * sd8","999 & 99.2","34 / 0"};





        for(String inputs : inputDatas){

            String inputArr = inputs.split("\s+");

            if(inputArr.length != 3){

                System.out.println("Invalid no of parameters, exact 3 accepting");

                continue;

            }



            if(numberValidator.validate(inputArr[0], inputArr[2]) && operatorValidator.validate(inputArr[1])){

                Calculator calculator = CalculatorFactory.getCalculator(inputArr[1]);

                if(calculator instanceof Division){

                    if(zeroValidator.validate(inputArr[2])){

                        System.out.println("Operand2 should not be 0 or 0.0 for Division");

                        continue;

                    }

                } 

                double result = calculator.calculate(Double.parseDouble(inputArr[0]), Double.parseDouble(inputArr[2]));

                System.out.println(inputs + "=" + result);

            } else{

                System.out.println("Invalid inputs : "+inputs);

            }



        }

    }

}

package com.calculator;



public interface Calculator {



    double calculate(double userInput1, double userInput2);

}

package com.calculator;



public class Addition implements Calculator{



    @Override

    public double calculate(double userInput1, double userInput2) {

        return userInput1 + userInput2;

    }



}

package com.calculator;



public class Substraction implements Calculator{



    @Override

    public double calculate(double userInput1, double userInput2) {

        return userInput1 - userInput2;

    }



}

package com.calculator;



public class Multiplication implements Calculator{



    @Override

    public double calculate(double userInput1, double userInput2) {

        return userInput1 * userInput2;

    }



}

package com.calculator;



public class Division implements Calculator{



    @Override

    public double calculate(double userInput1, double userInput2) {

        return userInput1/userInput2;

    }



}

package com.validator;



public interface Validator {

    boolean validate(String ... dataToBeValidate);

}

package com.validator;



import java.util.regex.Pattern;



public class NumberValidator implements Validator{



    @Override

    public boolean validate(String... dataToBeValidate){

        for(String data : dataToBeValidate){

            if(!(Pattern.matches("\d+", data) || Pattern.matches("\d+.?\d+", data))){

                return false;

            }

        }

        return true;

    }





}

package com.validator;



import java.math.BigDecimal;



public class ZeroValueValidator implements Validator{



    @Override

    public boolean validate(String... dataToBeValidate){

        for(String data : dataToBeValidate){

            if(BigDecimal.ZERO.compareTo(new BigDecimal(data)) != 0){

                return false;

            }

        }

        return true;

    }



}

package com.validator;



import java.util.Arrays;

import java.util.List;



public class OperatorValidator implements Validator{



    List<String> operatorList = Arrays.asList("+","-","/","*");

    @Override

    public boolean validate(String... dataToBeValidate){

        for(String data : dataToBeValidate){

            if(!operatorList.contains(data.trim())){

                return false;

            }

        }

        return true;

    }

}

package com.factory;



import com.calculator.Addition;

import com.calculator.Calculator;

import com.calculator.Division;

import com.calculator.Multiplication;

import com.calculator.Substraction;



public class CalculatorFactory {

    private static final Addition addition = new Addition();

    private static final Substraction substraction = new Substraction();

    private static final Multiplication multiplication = new Multiplication();

    private static final Division division = new Division();

    public static Calculator getCalculator(String operator){

         switch (operator) {

            case "+": return addition;

            case "-": return substraction;

            case "*": return multiplication;

            case "/": return division;

         }

        return null;

    }

}

My first impression was: wow, great test cases. This is something I often miss when looking at beginner's code. Your tests cover each line of code, except for one in the CalculatorFactory:

return null;

I had expected that the test case "999 & 99.2" would test this line, but I was wrong. This looked strange to me, and indeed you have some code duplication:

// OperatorValidator

List<String> operatorList = Arrays.asList("+","-","/","*");



// CalculatorFactory

switch (operator) {

    case "+": return addition;

    case "-": return substraction;

    case "*": return multiplication;

    case "/": return division;

}

Because of this duplication, you have to adjust two places whenever you add a new operator to the calculator. This can be done better. The OperatorValidator could simply check CalculatorFactory.getCalculator(operator) == null. Then you can delete the operatorList.

After this change, your code has 100% test coverage, which is impressing. And not only this, the tests also cover many interesting cases from the application domain, which is even more important.

Package names

You have chosen com.calculator and com.validator as package names. You should not use these names unless you own the DNS domains calculator.com and validator.com. Choosing good package names becomes important when you write software that others want to reuse. It's just too easy that someone else has also chosen the name com.calculator, therefore you should pick a more unique package name, maybe based on your name, or the name of your school, company or university.

Class names

I don't like the name Calculator that you gave your interface. A calculator can usually perform several different operations, like addition, subtraction, division. Your interface has only a single method. It therefore should be called Operation. And if you want to be very precise, it is a BinaryOperation since it takes two arguments. Contrast this with SquareRoot, which would be a UnaryOperation.

Substraction should be Subtraction (substract is obsolete).

Variable names

• 
  dataToBeValidate should be dataToBeValidated, with a d at the end


• 
  userInput1 and userInput2 don't necessarily come from the user

Input validation

You did a great job here. I tried to trick your calculator of performing a division by zero when I entered 123 / 0e0, but your validation in ZeroValueValidator caught this attempt.

The regular expression d+.?d+ in the NumberValidator is wrong, though. It accepts 123x456 because the . stands for an arbitrary character. You have two choices here:

Pattern.matches("\d+\.?\d+", data)

Pattern.matches("\d+[.]?\d+", data)

CalculatorFactory

In real life, when you go to a factory and ask for something, you always get a new thing. This is not the case for your CalculatorFactory. Therefore it should rather be called OperatorRegistry.

Source code layout

In a few places, your code layout is inconsistent. The indentation is sometimes 3 spaces, most often 4 and sometimes 5. With a good IDE, you should not need to do the indentation yourself, just let the IDE do that.

• Eclipse: Ctrl+Shift+F
  


• IntelliJ: Ctrl+Alt+L
  


• Visual Studio Code: Shift+Alt+F
  

Extensibility

You separated the code in nice, small chunks. Adding a new binary operation like "x to the power of y" or "gcd(x, y)" should be very easy now.

A more tricky step is to add support for unary operations:

• negate


• square root


• square


• reciprocal


• natural logarithm


• base-10 logarithm


• factorial

This will require much more work for the first operator. But since you modeled the binary operators so well, it should be easy once you have the first operator working.

Automated testing

Currently when you run the tests you have to inspect the output manually to see whether the results are what you expect. You should rewrite the testing code so that all the good cases don't produce any output. That's much easier to check manually.

The next step in this direction is to write unit tests, for example with JUnit.

Summary

You wrote a very good piece of code. It can still be improved in some places, but it works well, handles the corner cases and has good test data. I could criticize some more things, but I think it's enough for now. After you have improved your code, you are welcome to post a follow-up question here on this site.

I have a few comments to say about the main() method. It is doing the following:

1. define the test data


2. define all possible validators and initialize all of them upfront.


3. validate the input by applying validators


4. it knows when a validator should be applied


5. somewhere inside the validation code block, the method decides that it is now time to do the calculation


6. the method is also responsible for producing all the output (from validation as well as from calculation)

so we see that the method is responsible for quite a lot of processing. This has several issues, the most obvious one is breach of the Single responsibility principle. There are other issues here that need addressing:

1. this calculator is not very useful for the user since it cannot take any user input. test data, as well as "real" user input, should come as run time input. this may be from the command line, from an API call or from some kind of UI. havong the input specified at run time will allow you to modify the test data without need for recompile.




2. if ZeroValueValidator is applicable only for division, then perhaps the operator needs to determine which Validators to apply? another approach can be that each Validator can say whether it is applicable for a given calculation? so I believe the order of validation procesing should be: first parse the String expression into an Expression instance that holds operand/s and operator/s (that can perhaps be defined as enum?). then apply further "mathematical" validation.




3. there should be a clear separation between the validation and calculation steps. This will make things clearer and also allow for modifications to one step without affecting the other.




4. you should consider having the validators throw a custom exception. this is a perfect fit for that mechanism. Java even have some predefined exceptions that you cen reuse like IllegalArgumentException and ArithmeticException. the benefit of throwing java predefined exceptions is that they already have documentation and users of the calculator will be able to know what went wrong.




5. other areas for separation of concerns can be output production, validator instantiation, etc.