Object-oriented calculator

Question

I have solved an assignment (Exercise 3) from the MOOC Object-Oriented programming with Java, part II, but I'm not enrolled in said course.

Assignment Summary:

Make a simple calculator. You may create a Reader class that encapsulates the Scanner-object in order to communicate with the user. The Scanner-object can be an instance variable. Implement class Reader with the following methods:

• public String readString()
• public int readInteger()

To take care of the application logic of your program, a method public void start() may be used. The calculator has the commands sum, difference, product and end. Add an instance variable of the type Reader for the calculator and create the reader in the constructor. You may also implement a method private void statistics(). The method is meant to print the amount of operations done with the Calculator-object. If an invalid command is given (something other than sum, difference, product or end), the calculator should not react to the command in any way, but should instead continue by asking the next command. Statistics should not count as an invalid command as a completed calculation.

Changes made from assignment

• The Reader class doesn't exactly encapsulate the Scanner-object, and I have only provided static methods to the class. I had wanted it to inherit from the Scanner class, but Scanner was a final class.
• Scanner-object wasn't made an instance variable.
• enum is used for the various calculator commands.
• The calculator gives a message when an invalid command is given.
• The calculator contains only static methods.
• I have omitted method: public void start()
• A static variable : public static boolean isRunning(), is used a state variable

How do you refactor this code so that it follows OOP, reads better, is manageable? How can I write method names and classes better? How do you know which entities are to be made separate classes, and how to use classes efficiently?

I had thought of using a separate Command class that has a list of commands to use (whether in calculator or in any other class that uses a command style execution, like a terminal), but I didn't know how to proceed with that. Functions aren't objects in Java and generalizing seems difficult even with using Reflection in Java, especially the number and type of the return values, and typecasting new parameters with said types.

Reader Class used to simplify Scanner class (It can't be inherited, it seems)

import java.util.Scanner; public class Reader { public static String readString(String prompt) { System.out.print(prompt); return new Scanner(System.in).nextLine(); } public static int readInteger(String prompt) { return Integer.parseInt(readString(prompt)); } } 

Calculator class

import java.util.stream.IntStream; public class Calculator { private static int statistics = 0; private static boolean isRunning = true; enum Command { SUM, DIFFERENCE, PRODUCT, END } public static boolean isRunning() { return isRunning; } public static Command getCommand() throws IllegalCommand { String command = Reader.readString("command: "); try { return Command.valueOf(command.toUpperCase()); } catch (Exception e) { throw new IllegalCommand("Command " + command + " not found"); } } private static void printResult(String operation, int result) { System.out.println(operation + " of the values " + result); } private static int[] readOperands(int noOfOperands) { int[] array = new int[noOfOperands]; for (int i = 0; i < noOfOperands; i++) { array[i] = Reader.readInteger(String.format("value%d: ", i + 1)); } return array; } public static int sum(int... a) { return IntStream.of(a).sum(); } public static int product(int... a) { int result = 1; for (int i = 0; i < a.length; i++) { result *= a[i]; } return result; } public static void execute(Command command) { switch (command) { case SUM: { int[] operands = readOperands(2); printResult("Sum", sum(operands)); statistics++; break; } case DIFFERENCE: { int[] operands = readOperands(2); printResult("Difference", operands[0] - operands[1]); statistics++; break; } case PRODUCT: { int[] operands = readOperands(2); printResult("Product", product(operands)); statistics++; break; } case END: { System.out.println("Calculations done " + statistics); isRunning = false; break; } } } } 

Main

public class Main { public static void main(String[] args) { while (Calculator.isRunning()) { try { Calculator.execute(Calculator.getCommand()); System.out.println(); } catch(IllegalCommand e) { System.out.println(e); } } } } 

IllegalCommand , a custom Exception class. Is this an overkill? I just wanted to give it a type, in case it clashes with other Exception types in the future.

public class IllegalCommand extends Exception { public IllegalCommand(String s) { super(s); } } 

Expected output

command: sum value1: 4 value2: 6 Sum of the values 10 command: product luku1: 3 luku2: 2 Product of the values 6 command: integral IllegalCommand: Command integral not found command: difference value1: 3 value2: 2 Difference of the values 1 command: end Calculations done 3 

Answer 1

new Scanner(System.in).nextLine() 

This is wasteful. Creating a new scanner object every time that you read a line is very bad for performance. Not to mention that you never close it.

Why does your Calculator get commands and read STDIN? Your calculator class should do one thing: calculate. It is after all, a calculator.

Maybe make a UserInputHandler class if you want to take the input handling out of the main class.

public class UserInputHandler implements AutoCloseable { private Scanner scanner; private Boolean isRunning = false; class Commands { public static final String SUM = "sum"; public static final String DIFFERENCE = "difference"; public static final String PRODUCT = "product"; public static final String END = "end"; } public UserInputHandler() { this(new Scanner(System.in)); } public UserInputHandler(Scanner scanner) { this.scanner = scanner; } public boolean isRunning() { return isRunning; } public void startBlocking() { isRunning = true; startHandlingInput(); } public void stop() { isRunning = false; } private void startHandlingInput() { while (isRunning) { System.out.print("Command: "); if (scanner.hasNext()) handleCommand(scanner.nextLine()); } } private void handleCommand(String input) { switch(input) { case Commands.SUM: System.out.printf("Result: %s\n", Calculator.sum(readOperands(2))); break; case Commands.DIFFERENCE: System.out.printf("Result: %s\n", Calculator.subtract(readOperands(2))); break; case Commands.PRODUCT: System.out.printf("Result: %s\n", Calculator.product(readOperands(2))); break; case Commands.END: stop(); break; default: System.out.println("Unknown command!"); } } private int[] readOperands(int noOfOperands) { int[] array = new int[noOfOperands]; for (int i = 0; i < noOfOperands; i++) { System.out.printf("value %d: ", i+1); String nextLine = scanner.nextLine(); try { int nextInt = Integer.parseInt(nextLine); array[i] = nextInt; } catch (NumberFormatException e) { System.out.println("Invalid number, please enter again."); i--; } } return array; } @Override public void close() throws IOException { scanner.close(); } } 

Don't use Enum#valueOf using exceptions in control flow is a terrible idea.

Your Calculator can be simple. Also, why use streams for some things and not others?

public class Calculator { private static int statistics = 0; public static int subtract(int... a) { statistics++; return a[0] - IntStream.of(a) .skip(1) .sum(); } public static int sum(int... a) { statistics++; return IntStream.of(a).sum(); } public static int product(int... a) { statistics++; return IntStream.of(a) .reduce((x, y) -> x * y) .orElseThrow(RuntimeException::new); } } 

Now just run your app:

public class Main { public static void main(String[] args) { try (UserInputHandler handler = new UserInputHandler()) { handler.startBlocking(); } catch (IOException e) { e.printStackTrace(); } } } 

Simple!