Simple object oriented console calculator

I'd like to learn OOP. Could someone check my code and tell me how to improve it?

package com.company;

import java.util.InputMismatchException;
import java.util.Scanner;

public class CalculatorApp {
private static Scanner scanner = new Scanner(System.in);
public static void main(String[] args) {
System.out.println("Write two numbers and +, -, * or / sign");
char operator = 0;
double firstNum = 0;
double secondNum = 0;
try {
firstNum = scanner.nextDouble();
secondNum = scanner.nextDouble();
operator = scanner.next().charAt(0);
} catch (InputMismatchException ime) {
System.out.println("invalid input");
} finally {
if (operator != '+' & operator != '-' & operator != '*' & operator != '/') {
throw new InputMismatchException();
}
Calculator calculator = new Calculator(firstNum, secondNum,    operator);
System.out.println(calculator.makeCalculation());
}
}
}


Calculator class:

package com.company;

import java.util.HashMap;
import java.util.Map;

public class Calculator {
private char operation;
private double operand1;
private double operand2;
private Map<Character, Operation> operationMap = new HashMap<>();

public Calculator(double operand1, double operand2, char operation) {
this.operand1 = operand1;
this.operand2 = operand2;
this.operation = operation;

operationMap.put('-', new Subtraction());
operationMap.put('*', new Multiplication());
operationMap.put('/', new Division());
}

public double makeCalculation() {
Operation operationMapValue = null;
if (operationMap.containsKey(operation)) {
operationMapValue = operationMap.get(operation);
System.out.println(operationMap.get(operation));
} else {
System.out.println("Invalid sign");
}
return operationMapValue.calculateResult(operand1, operand2);
}
}


Operation interface:

package com.company;

public interface Operation {
double calculateResult(double left, double right);
}


One of implementing classes:

package com.company;

public class Addition implements Operation {
@Override
public double calculateResult(double left, double right) {
return left + right;
}
}


And Tests:

package com.company;

import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.*;

class CalculatorTest {
Calculator calc = new Calculator(6.0, 2.0, '+');

@Test
assertEquals(8.0, calc.makeCalculation());
}

Calculator calc2 = new Calculator(6.0, 2.0, '-');
@Test
void subtractsTwoNumbers() {
assertEquals(4.0, calc2.makeCalculation());
}

Calculator calc3 = new Calculator(6.0, 2.0, '*');
@Test
void multipliesTwoNumbers() {
assertEquals(12.0, calc3.makeCalculation());
}

Calculator calc4 = new Calculator(6.0, 2.0, '/');
@Test
void dividesTwoNumbers() {
assertEquals(3.0, calc4.makeCalculation());
}
}

• Just a little tip: since your interface only has one method, you can declare it as a @FunctionalInterface; this way, if you are in Java 8, you can define it with a lambda. Commented Jul 15, 2019 at 14:18
• Don't call it Calculator, call it Calculation instead, here is why: yegor256.com/2015/03/09/objects-end-with-er.html Commented Jun 5, 2023 at 1:55

Looks good in many aspects!

Thumbs up for the Operation interface and its implementations.

Thumbs up for the JUnit tests.

Thumbs up for the operationMap.

What I'd do differently is the cardinality / life cycle of the Calculator instance(s).

You are creating a new Calculator for every calculation you are doing (and together with the Calculator, new Operations).

I'd have one Calculator instance, filling the operationMap the way you did, but not taking the double operand1, double operand2, char operation parameters in the constructor. I'd move them to the makeCalculation() method, changing that to makeCalculation(double operand1, double operand2, char operation).

This way the Calculator's initialization needs to run only once, and then he's ready for as many calculations as you want - just like a hardware calculator that you typically use for more than one calculation. And there's no longer a need for the Calculator to permanently store the operand1, operand2, and operation as fields.

And some details you could improve:

• You're mixing computation and output (System.out.println() ) in makeCalculation(). That makes your otherwise good calculator unsusable in e.g. a GUI or a web application.

• The error handling in makeCalculation() should throw an exception if something goes wrong instead of printing something to System.out. This tells your caller that you weren't able to compute a result.

• When called with an invalid operation, makeCalculation() might run into a NullPointerException although you try to handle that in your if construct.

Exception handling

try {
...
} catch (InputMismatchException ime) {
System.out.println("invalid input");
} finally {
if (operator != '+' & operator != '-' & operator != '*' & operator != '/') {
throw new InputMismatchException();
}
Calculator calculator = new Calculator(firstNum, secondNum,    operator);
...
}


That's pretty weird.

It makes no sense to continue the computation if the Scanner throws. What is the point of having a finally block that throws the same exception? I would rather let the exception pass through instead of catching and rethrowing (the original exception is likely to contain a more meaningful message, too).

Variable scope

Try and use the narrowest possible scope. For instance, here:

Calculator calc = new Calculator(6.0, 2.0, '+');

@Test
assertEquals(8.0, calc.makeCalculation());
}


there's no point in making calc a field. It can (and should) be just a local variable in the addsTwoNumbers() method. The same is true for other calc's and test methods.

Class design

I don't think that the Calculator is a good name for your class. I would expect a calculator to be able to be instantiated once and then evaluate results of different expression. Something like this:

Calculator calculator = new Calculator();
double sum = calculator.evaluateExpression(x, y, '+');
double difference = caclulator.evaluateExpression(x, y, '-');


I'd change it this way or rename this class to Expression.

Object reuse

Creating four new objects for each operation per each expression evaluation can be costly and is unreasonable because the operation are stateless.

You can use a static factory to reuse the same Operation objects in different expressions.

Is it ok now?

public class CalculatorApp {
private static Scanner scanner = new Scanner(System.in);
public static void main(String[] args) {
System.out.println("Write two numbers and +, -, * or / sign");
char operator = 0;
double firstNum = 0;
double secondNum = 0;
try {
firstNum = scanner.nextDouble();
secondNum = scanner.nextDouble();
operator = scanner.next().charAt(0);
} catch (InputMismatchException ime) {
System.out.println("invalid input");
}
Calculator calculator = new Calculator();
calculator.makeCalculation(firstNum, secondNum, operator);
}
}


Calculator class:

class Calculator {
private Map<Character, Operation> operationMap = new HashMap<>();
Calculator() {
operationMap.put('-', new Subtraction());
operationMap.put('*', new Multiplication());
operationMap.put('/', new Division());
}

double makeCalculation(double operand1, double operand2, char operation) {
Operation operationMapValue = operationMap.getOrDefault(operation,new NoOperationFound(operation));
return operationMapValue.calculateResult(operand1, operand2);
}
}


Interface:

public interface Operation {
double calculateResult(double left, double right);
}


Implementing class:

public class Addition implements Operation {
@Override
public double calculateResult(double left, double right) {
return left + right;
}
}


NoOperationFound class:

public class NoOperationFound implements Operation {
private char sign;
NoOperationFound(char sign){
this.sign = sign;
}

@Override
public double calculateResult(double left, double right) {
throw new InputMismatchException("Invalid operator sign: " + sign);
}
}


Tests:

class CalculatorTest {
private Calculator calculator = new Calculator();

@Test
assertEquals(8.0, calculator.makeCalculation(6.0,2.0,'+'));
}

@Test
void subtractsTwoNumbers() {
assertEquals(4.0, calculator.makeCalculation(6.0, 2.0, '-'));
}

@Test
void multipliesTwoNumbers() {
assertEquals(12.0, calculator.makeCalculation(6.0, 2.0, '*'));
}

@Test
void dividesTwoNumbers() {
assertEquals(3.0, calculator.makeCalculation(6.0, 2.0, '/'));

}
}

• Thumbs up! I've seen "seasoned" OOP developers doing worse... Commented Sep 13, 2017 at 19:48

But Id like to add something:

defensive coding

Your code risks to throw NullPointerExceptions:

    Operation operationMapValue = null;
if (operationMap.containsKey(operation)) {
operationMapValue = operationMap.get(operation);
System.out.println(operationMap.get(operation));
} else {
System.out.println("Invalid sign");
}
return operationMapValue.calculateResult(operand1, operand2);


In the case of an "invalid sign" the last line gets executed an an NPE is thrown because the variable operationMapValue still points to null.

You should have written it this way:

public class NoOperationFound implements Operation {
@Override
public double calculateResult(double left, double right) {
throw new InputMismatchException("invalid operator sign");
}
}

• use the getOrDefault() method of Map (in java 8)

  Operation operationMapValue = operationMap.getOrDefault(operation,new NoOperationFound());
return operationMapValue.calculateResult(operand1, operand2);


This will throw the expected exception without having to check explicitly.

In this simple version you could have a constant of NoOperationFound as suggested by @kraskevich but with a little variation the error message could be clearer:

    public class NoOperationFound implements Operation {
private final char sign;
public NoOperationFound(char sign){
this.sign = sign;
}
@Override
public double calculateResult(double left, double right) {
throw new InputMismatchException("invalid operator sign : "+sign);
}
}

Operation operationMapValue =
operationMap.getOrDefault(operation,new NoOperationFound(operation));


It is also applicable by using the strategy pattern.

You can use the operations(+,-,/,*) by strategy. And you can give this strategy as a context to the calculate method.

package OOPDesign.calculator;

public interface CalculationStrategy {

int calculate(int value1, int value2);
}


package OOPDesign.calculator;

public class AdditionStrategy implements CalculationStrategy {

@Override
public int calculate(int value1, int value2) {
return value1 + value2;
}
}


SubtractionStrategy

package OOPDesign.calculator;

public class SubtractionStrategy implements CalculationStrategy {

@Override
public int calculate(int value1, int value2) {
return value1 - value2;
}
}


Calculator

package OOPDesign.calculator;

public class Calculator {

public static Calculator instance = null;

CalculationStrategy calculationStrategy;

public void setCalculationStrategy(CalculationStrategy calculationStrategy) {
this.calculationStrategy = calculationStrategy;
}

public static Calculator getInstance(){
if(instance == null){
instance = new Calculator();
}
return instance;
}

public int calculate(int value1, int value2) {
return calculationStrategy.calculate(value1, value2);
}
}


and Test class

package OOPDesign.calculator;

public class CalculatorMain {

public static void main(String[] args) {

Calculator c = Calculator.getInstance();

System.out.println(c.calculate(5 ,2));

c.setCalculationStrategy(new SubtractionStrategy());
System.out.println(c.calculate(5 ,2));

c.setCalculationStrategy(new MultiplicationStrategy());
System.out.println(c.calculate(5 ,2));

c.setCalculationStrategy(new DivideStrategy());
System.out.println(c.calculate(5 ,2));
}
}

• Welcome to code review where we review working code to provide suggestions on how to improve code. While the answer shows a lot of knowledge about design patterns and programming, it provides an alternate solution rather than reviewing the code. What insightful observation can you make about the code itself? Answers that only provide alternate solutions may be down voted and deleted. This would be a very good answer on stackoverflow.com, but doesn't really provide a code review. Commented Dec 31, 2019 at 15:30