5
\$\begingroup\$

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 Addition());
        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
    void addsTwoNumbers() {
        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());
    }
}
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – eric.m yesterday
4
\$\begingroup\$

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.

\$\endgroup\$
3
\$\begingroup\$

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
void addsTwoNumbers() {
     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.

\$\endgroup\$
1
\$\begingroup\$

Thanks for sharing your code.

I fully agree with the answers of Ralf and @kraskevich.

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:

  • create a special instead of null implementation of your interface:

    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));
\$\endgroup\$
1
\$\begingroup\$

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 Addition());
        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
    void addsTwoNumbers() {
        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, '/'));

    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Thumbs up! I've seen "seasoned" OOP developers doing worse... \$\endgroup\$ – Ralf Kleberhoff Sep 13 '17 at 19:48

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.