Calculator with history using Java 8

Question

Yesterday, I saw this post from another user: Calculator with history

I started refactoring the code while typing an answer but I end up not posting as an answer because I not sure if my implementation was good enough because of the difficulty I found to separate the concerns in that example.

I created my own implementation and tried to use new Java 8 features as much I as could and clean code practices (although I don't have much experience with these two).

I create some unit tests to ensure that the program works as the original. I will not post all the tests here, but I put it in a git repository as well as the project files: tests, project

import java.util.Scanner;

public class RunProgram {

    public static void main(String[] args) {
        Calculator calculator = new Calculator(new Solver(), new Scanner(System.in));
        calculator.run();

    }
}

---

import java.util.Optional;
import java.util.Scanner;

public class Calculator {

    public static final String EXIT_MESSAGE = "Okay! Nobody misses you" +
            "\n But here's the calculations you've done so far";
    private static final String WRONG_ACTION_MESSAGE = "Shit, wrong answer,  you'll have to calculate " +
            "with these numbers again and then you can do whatever you want";
    private HistoryHolder historyHolder = new HistoryHolder();
    private Solver solver;
    private Scanner scanner;

    private AskForAction askForAction;
    private AskForNumber askForFirstNumber;
    private AskForNumber askForSecondNumber;
    private AskForOperation askForOperation;
    private double firstNumber;
    private double secondNumber;

    public Calculator(Solver solver, Scanner scanner) {
        this.scanner = scanner;
        this.solver = solver;
        initializeAskers();
    }

    private void initializeAskers() {
        askForAction = new AskForAction(this.scanner);
        askForFirstNumber = new AskForNumber("Input the 1st number", this.scanner);
        askForSecondNumber = new AskForNumber("Input the 2nd number", this.scanner);
        askForOperation = new AskForOperation(this.scanner);
    }

    public void run() {
        askUsersUnitialNumbers();
        Action action;
        do {
            Expression expression = buildExpression();
            processExpression(expression);
            action = askAndVerifyAction();
            if (action.isChangeNumbers()) {
                askUsersUnitialNumbers();
            }
        } while (!action.isStopCondition());
        endProgram();
    }

    private void askUsersUnitialNumbers() {
        firstNumber = askForFirstNumber.ask();
        secondNumber = askForSecondNumber.ask();
    }

    private Expression buildExpression() {

        char operator = askForOperation.ask();

        return Expression.Builder.anExpression()
                .firstNumber(firstNumber)
                .secondNumber(secondNumber)
                .operator(operator).build();
    }

    private void endProgram() {
        printExitMessage();
        printHistory();
    }

    private Action askAndVerifyAction() {
        Optional<Action> possibleAction = askForAction.ask();
        verifyPossibleAction(possibleAction);
        return possibleAction.get();
    }

    private void verifyPossibleAction(Optional<Action> possibleAction) {
        if (!possibleAction.isPresent()) {
            showWrongActionMessage();
        }
    }


    private void processExpression(Expression expression) {
        Optional<Double> possibleResult = solver.solveExpression(expression);

        printResult(possibleResult);
        addToHistory(possibleResult);
    }


    private void showWrongActionMessage() {
        System.out.println(WRONG_ACTION_MESSAGE);
    }

    private void printExitMessage() {
        System.out.println(EXIT_MESSAGE);
    }

    private void addToHistory(Optional<Double> pretendResult) {
        if (pretendResult.isPresent()) {
            historyHolder.add(pretendResult.get());
        }
    }

    private void printResult(Optional<Double> pretendResult) {
        if (pretendResult.isPresent()) {
            System.out.println(pretendResult.get());
        }
    }

    private void printHistory() {
        historyHolder.printHistory();
    }

}

---

import java.util.Arrays;
import java.util.Optional;

public enum Action {

    DO_MORE_CALCULATIONS(1), EXIT_AND_PRINT_HISTORY(2), CHANGE_NUMBERS(3);

    private int actionNumber;

    Action(int actionNumber) {
        this.actionNumber = actionNumber;
    }

    public static Optional<Action> byActionNumber(int actionNumber) {
        return Arrays.stream(values())
                .filter(action -> action.actionNumber == actionNumber)
                .findFirst();
    }

    public boolean isStopCondition() {
        return this == EXIT_AND_PRINT_HISTORY;
    }

    public boolean isChangeNumbers() {
        return this == CHANGE_NUMBERS;
    }
}

---

import java.util.Optional;
import java.util.Scanner;
import java.util.function.BiFunction;

public class AskForAction {

    private static final BiFunction<String, Scanner, Optional<Action>> READ_ACTION = (menuMessage, scanner) -> {
        System.out.println(menuMessage);
        String actionString = scanner.next();
        return NumberUtils.isValidInt(actionString) ? Action.byActionNumber(Integer.parseInt(actionString)) : Optional.empty();
    };

    private static final String CHOOSE_ACTION_MENU = "Dormammu, I came to bargain! " +
            "Wanna do some extra calculations?" +
            "\n 1 - for 'Yes'" +
            "\n 2 - for 'No'" +
            "\n 3 - to change the numbers";

    private Scanner scanner;

    public AskForAction(Scanner scanner) {
        this.scanner = scanner;
    }

    public Optional<Action> ask() {
        return READ_ACTION.apply(CHOOSE_ACTION_MENU, scanner);
    }
}

---

import java.util.InputMismatchException;
import java.util.Scanner;
import java.util.function.BiFunction;

public class AskForNumber {
    private static final BiFunction<String, Scanner, Double> READ_NUMBER_FUNCTION = (menuMessage, scanner) -> {
        System.out.println(menuMessage);
        String input = scanner.next();
        if (NumberUtils.isValidFloat(input)) {
            return Double.valueOf(input);
        }
        throw new InputMismatchException("not a valid number!");
    };

    private final String menuMessage;
    private final Scanner scanner;


    public AskForNumber(String menuMessage, Scanner scanner) {
        this.menuMessage = menuMessage;
        this.scanner = scanner;
    }

    public double ask() {
        return READ_NUMBER_FUNCTION.apply(menuMessage, scanner);
    }

}

---

import java.util.Scanner;
import java.util.function.BiFunction;

public class AskForOperation {
    static final BiFunction<String, Scanner, Character> READ_OPERATION = (menuMessage, scanner) -> {
        System.out.println(menuMessage);
        String operation = scanner.next();
        return operation.charAt(0);
    };

    private static final String OPERATION_MENU = "What to do?" +
            "\n + for add" +
            "\n - for minus" +
            "\n * for multiply" +
            "\n / for divide" +
            "\n % for mod" +
            "\n ^ for first number into the power of second number";

    private Scanner scanner;

    public AskForOperation(Scanner scanner) {
        this.scanner = scanner;
    }

    public Character ask() {
        return READ_OPERATION.apply(OPERATION_MENU, scanner);
    }
}

---

public class Expression {
    private char checker;
    private double firstNumber;
    private double secondNumber;

    private Expression(char checker, double firstNumber, double secondNumber) {
        this.checker = checker;
        this.firstNumber = firstNumber;
        this.secondNumber = secondNumber;
    }

    public char checker() {
        return checker;
    }

    public double firstNumber() {
        return firstNumber;
    }

    public double secondNumber() {
        return secondNumber;
    }

    public static final class Builder {
        private char checker;
        private double firstNumber;
        private double secondNumber;

        private Builder() {
        }

        public static Builder anExpression() {
            return new Builder();
        }

        public Builder operator(char checker) {
            this.checker = checker;
            return this;
        }

        public Builder firstNumber(double firstNumber) {
            this.firstNumber = firstNumber;
            return this;
        }

        public Builder secondNumber(double secondNumber) {
            this.secondNumber = secondNumber;
            return this;
        }

        public Expression build() {
            Expression expression = new Expression(checker, firstNumber, secondNumber);
            return expression;
        }
    }
}

---

import java.util.ArrayList;
import java.util.List;

public class HistoryHolder {
    private List<Double> history = new ArrayList<>();

    public void add(Double number) {
        history.add(number);
    }

    public void printHistory() {
        history.forEach(number -> {
            System.out.print(number + " ");
        });

    }
}

---

import java.util.function.IntPredicate;

public class NumberUtils {
    static boolean isValidInt(String action) {
        return action.chars().allMatch(Character::isDigit);
    }

    static boolean isValidFloat(String string) {
        IntPredicate p = (x) -> x == '.';
        return string.chars().allMatch(p.or(Character::isDigit));
    }
}

---

import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.function.BiFunction;

public class Solver {
    private static final BiFunction<Double, Double, Double> SUM = (firstNumber, secondNumber) -> firstNumber + secondNumber;
    private static final BiFunction<Double, Double, Double> SUB = (firstNumber, secondNumber) -> firstNumber - secondNumber;
    private static final BiFunction<Double, Double, Double> MUL = (firstNumber, secondNumber) -> firstNumber * secondNumber;
    private static final BiFunction<Double, Double, Double> DIV = (firstNumber, secondNumber) -> firstNumber / secondNumber;
    private static final BiFunction<Double, Double, Double> MOD = (firstNumber, secondNumber) -> firstNumber % secondNumber;
    private static final BiFunction<Double, Double, Double> POW = (firstNumber, secondNumber) -> Math.pow(firstNumber, secondNumber);
    private static Map<Character, BiFunction<Double, Double, Double>> map;

    static {
        map = new HashMap<>();
        map.put('+', SUM);
        map.put('-', SUB);
        map.put('*', MUL);
        map.put('/', DIV);
        map.put('%', MOD);
        map.put('^', POW);
    }

    public Optional<Double> solveExpression(Expression expression) {
        BiFunction<Double, Double, Double> operation = map.get(expression.checker());
        if (operation == null) {
            return Optional.empty();
        }
        return Optional.of(operation.apply(expression.firstNumber(), expression.secondNumber()));
    }

}

---

The mainly test class:

import org.hamcrest.Matcher;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

import java.io.ByteArrayOutputStream;

import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;

public class CalculatorTest {

    public static final String OUTPUT_EXPECTED_TEST_1 = "Input the 1st number\n" +
            "Input the 2nd number\n" +
            "What to do?\n" +
            " + for add\n" +
            " - for minus\n" +
            " * for multiply\n" +
            " / for divide\n" +
            " % for mod\n" +
            " ^ for first number into the power of second number\n" +
            "3.0\n" +
            "Dormammu, I came to bargain! Wanna do some extra calculations?\n" +
            " 1 - for 'Yes'\n" +
            " 2 - for 'No'\n" +
            " 3 - to change the numbers\n" +
            "Okay! Nobody misses you\n" +
            " But here's the calculations you've done so far\n" +
            "3.0";
    public static final String OUTPUT_EXPECTED_TEST_2 = "Input the 1st number\n" +
            "Input the 2nd number\n" +
            "What to do?\n" +
            " + for add\n" +
            " - for minus\n" +
            " * for multiply\n" +
            " / for divide\n" +
            " % for mod\n" +
            " ^ for first number into the power of second number\n" +
            "3.0\n" +
            "Dormammu, I came to bargain! Wanna do some extra calculations?\n" +
            " 1 - for 'Yes'\n" +
            " 2 - for 'No'\n" +
            " 3 - to change the numbers\n" +
            "What to do?\n" +
            " + for add\n" +
            " - for minus\n" +
            " * for multiply\n" +
            " / for divide\n" +
            " % for mod\n" +
            " ^ for first number into the power of second number\n" +
            "1.0\n" +
            "Dormammu, I came to bargain! Wanna do some extra calculations?\n" +
            " 1 - for 'Yes'\n" +
            " 2 - for 'No'\n" +
            " 3 - to change the numbers\n" +
            "Okay! Nobody misses you\n" +
            " But here's the calculations you've done so far\n" +
            "3.0 1.0";
    public static final String OUTPUT_EXPECTED_TEST_3 = "Input the 1st number\n" +
            "Input the 2nd number\n" +
            "What to do?\n" +
            " + for add\n" +
            " - for minus\n" +
            " * for multiply\n" +
            " / for divide\n" +
            " % for mod\n" +
            " ^ for first number into the power of second number\n" +
            "3.0\n" +
            "Dormammu, I came to bargain! Wanna do some extra calculations?\n" +
            " 1 - for 'Yes'\n" +
            " 2 - for 'No'\n" +
            " 3 - to change the numbers\n" +
            "What to do?\n" +
            " + for add\n" +
            " - for minus\n" +
            " * for multiply\n" +
            " / for divide\n" +
            " % for mod\n" +
            " ^ for first number into the power of second number\n" +
            "1.0\n" +
            "Dormammu, I came to bargain! Wanna do some extra calculations?\n" +
            " 1 - for 'Yes'\n" +
            " 2 - for 'No'\n" +
            " 3 - to change the numbers\n" +
            "Input the 1st number\n" +
            "Input the 2nd number\n" +
            "What to do?\n" +
            " + for add\n" +
            " - for minus\n" +
            " * for multiply\n" +
            " / for divide\n" +
            " % for mod\n" +
            " ^ for first number into the power of second number\n" +
            "-1.0\n" +
            "Dormammu, I came to bargain! Wanna do some extra calculations?\n" +
            " 1 - for 'Yes'\n" +
            " 2 - for 'No'\n" +
            " 3 - to change the numbers\n" +
            "Okay! Nobody misses you\n" +
            " But here's the calculations you've done so far\n" +
            "3.0 1.0 -1.0";
    Calculator calculator;
    private ByteArrayOutputStream output;

    @Before
    public void setUp() throws Exception {
        output = TestUtils.setSystemOutputAndReturnOutput();
    }

    @After
    public void tearDown() throws Exception {
        TestUtils.resetSystemOutput();

    }

    @Test
    public void runOneRound() throws Exception {
        String firstRoundInput = "1\n2\n+\n2\n";
        calculator = new Calculator(new Solver(), TestUtils.getScannerWithInput(firstRoundInput));
        calculator.run();
        assertNotNull(calculator);
        Matcher<String> expected = TestUtils.createIgnoreSeparatorMatcher(CalculatorTest.OUTPUT_EXPECTED_TEST_1);
        assertThat(output.toString(), expected);
    }

    @Test
    public void runTwoRoundsAddPowAndExit() throws Exception {
        String firstRoundInput = "1\n2\n+\n1\n^\n2\n";
        calculator = new Calculator(new Solver(), TestUtils.getScannerWithInput(firstRoundInput));
        calculator.run();
        assertNotNull(calculator);
        Matcher<String> expected = TestUtils.createIgnoreSeparatorMatcher(CalculatorTest.OUTPUT_EXPECTED_TEST_2);
        assertThat(output.toString(), expected);
    }

    @Test
    public void runTwoRoundsAddPowChangeNumbersSubExit() throws Exception {
        String firstRoundInput = "1\n2\n+\n1\n^\n3\n1\n2\n-\n2\n";
        calculator = new Calculator(new Solver(), TestUtils.getScannerWithInput(firstRoundInput));
        calculator.run();
        assertNotNull(calculator);
        Matcher<String> expected = TestUtils.createIgnoreSeparatorMatcher(CalculatorTest.OUTPUT_EXPECTED_TEST_3);
        assertThat(output.toString(), expected);
    }


}

---

import org.hamcrest.Matcher;
import org.hamcrest.Matchers;

import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.util.Scanner;

public class TestUtils {
    public static ByteArrayOutputStream setSystemOutputAndReturnOutput() {
        ByteArrayOutputStream outputContent = new ByteArrayOutputStream();
        PrintStream printStream = new PrintStream(outputContent);
        System.setOut(printStream);
        return outputContent;
    }

    public static void resetSystemOutput() {
        System.setOut(null);
    }


    static Scanner getScannerWithInput(String input) {
        return new Scanner(new ByteArrayInputStream(input.getBytes()));
    }

    static Matcher<String> createIgnoreSeparatorMatcher(String expected) {
        return Matchers.equalToIgnoringWhiteSpace(expected);
    }
}

Answer 1

General

I was struggling about making this code review as in the first place I saw "a lot" of classes and separations of concerns (so my guess). But I went deeper into the analysis. Finally I came to the conclusion that something is strange about it.

Do not get me wrong. You have basic modularization. But this has gone into only one dimension. You separated concerns vertically but not horizontally.

Action

The first thing which makes me think about is not beneficial polymorphism.

An indicator for unfavourable polymorphism is that no behaviour is encapsulated. The only method in your Action enum with something to do is "byActionNumber". And this method is not under polymorphism. "isStopCondition" and "isChangeNumbers" are unnecessary indirections. You can evaluate directly the type and it is worth the same. Maybe at least "isStopCondition" is adressing another aspect.

An inheritance hierarchy with each subclass providing same amount of boolean getters that will evaluate if the current instance is of a specific type makes this inheritance at least boiler-code. But furthermore you provide internal knowledge that you wanted to abstract in the first place.

Calculator

The Calculator has too many responsibilities even if it delegates tasks to its known "friends". Effectivly the Calculator "inherits" the responsibilities of all sub components.

So effectively your Calculator

... holds the process state

... gathers input from a scanner for process purposes

... produces intermediate output to the console for process purposes

... gathers input from a scanner for calculation purposes

... calculates (this is what a Calculator is all about)

... holds and maintains the history

... produces the calculation result as output

... produces the history as output

One reason for that: you haven't introduced abstractions at certain points. You are violating "dependency inversion principle".

You can easily figure out the resulting coupling and god-like responsibilities by asking following questions:

• Can the input process work, even if a Calculator is not present? No.
• Can the Calculator work without the Scanner be present? No.
• Can the Calculator work without the Console output possibility to be present? No.
• Can the Calculator work without the History present? No.

I provide an example that may be helpful to understand what I mean: See following code:

public static void main(String[] args) {

    Input input = new Input();
    Output output = new Output();

    Calculator calculator = new Calculator();

    output.writeToConsole("Input the 1st number");

    calculator.setFirstOperand(input.getNumberFromKeyboard());

    output.writeToConsole("What to do?" +
            "\n + for add" +
            "\n - for minus" +
            "\n * for multiply" +
            "\n / for divide" +
            "\n % for mod" +
            "\n ^ for first number into the power of second number");

    calculator.setOperator(input.getCharFromKeyboard());

    output.writeToConsole("Input the 2nd number");

    calculator.setSecondOperand(input.getNumberFromKeyboard());

    calculator.execute();

    output.writeToHistory(calculator.getCurrentResult());
    output.writeToConsole(Double.toString(calculator.getCurrentResult()));

}

Now we remove the Calculator:

public static void main(String[] args) {

    Input input = new Input();
    Output output = new Output();

    output.writeToConsole("Input the 1st number");

    input.getNumberFromKeyboard();

    output.writeToConsole("What to do?" +
            "\n + for add" +
            "\n - for minus" +
            "\n * for multiply" +
            "\n / for divide" +
            "\n % for mod" +
            "\n ^ for first number into the power of second number");

    input.getCharFromKeyboard();

    output.writeToConsole("Input the 2nd number");

    input.getNumberFromKeyboard();

}

Now we remove the Input:

public static void main(String[] args) {

    Output output = new Output();

    Calculator calculator = new Calculator();

    output.writeToConsole("Input the 1st number");

    calculator.setFirstOperand(5);

    output.writeToConsole("What to do?" +
            "\n + for add" +
            "\n - for minus" +
            "\n * for multiply" +
            "\n / for divide" +
            "\n % for mod" +
            "\n ^ for first number into the power of second number");

    calculator.setOperator("+");

    output.writeToConsole("Input the 2nd number");

    calculator.setSecondOperand(7);

    calculator.execute();

    output.writeToHistory(calculator.getCurrentResult());
    output.writeToConsole(Double.toString(calculator.getCurrentResult()));

}

Now we remove the Console Output:

public static void main(String[] args) {

    Input input = new Input();
    Output output = new Output();

    Calculator calculator = new Calculator();

    calculator.setFirstOperand(input.getNumberFromKeyboard());

    calculator.setOperator(input.getCharFromKeyboard());

    calculator.setSecondOperand(input.getNumberFromKeyboard());

    calculator.execute();

    output.writeToHistory(calculator.getCurrentResult());

}

Now we remove the History:

public static void main(String[] args) {

    Input input = new Input();
    Output output = new Output();

    Calculator calculator = new Calculator();

    output.writeToConsole("Input the 1st number");

    calculator.setFirstOperand(input.getNumberFromKeyboard());

    output.writeToConsole("What to do?" +
            "\n + for add" +
            "\n - for minus" +
            "\n * for multiply" +
            "\n / for divide" +
            "\n % for mod" +
            "\n ^ for first number into the power of second number");

    calculator.setOperator(input.getCharFromKeyboard());

    output.writeToConsole("Input the 2nd number");

    calculator.setSecondOperand(input.getNumberFromKeyboard());

    calculator.execute();

    // output.writeToHistory(calculator.getCurrentResult());
    output.writeToConsole(Double.toString(calculator.getCurrentResult()));

}

In any of these situations all other code is not touched. That is because this sequence only defines the process.

If you do this with your code some changes may be easier than others but all of them will have effects on either compilation units that also have other responsibilities and/or your changes will affect multiple compilation units to make it work again even if you want to change only one aspect.

Another indicator that you have high coupling is that you chose an integration test to test your Calculator. I do not say "do not make integration tests". I only wanted to mention that decisions made have reasons. These reasons may be valid or not. But for me integration tests are an indicator for less useful code separation.

AskFor...

These classes are wrappers for the Scanner. But they also assume that the console output is available. Introducing the console output will produce a second ouput beside the validated return value got from the console input.

Removing the console output will make these classes well-defined and reusable even if no console ouput is available. That doesn't affect providing a context message:

public class AskForNumber {
    private static final Function<Scanner, Double> READ_NUMBER_FUNCTION = (scanner) -> {
        // System.out.println(contextMessage);
        String input = scanner.next();
        if (NumberUtils.isValidFloat(input)) {
            return Double.valueOf(input);
        }
        throw new InputMismatchException("not a valid number!");
    };

    private final String contextMessage;
    private final Scanner scanner;


    public AskForNumber(String contextMessage, Scanner scanner) {
        this.contextMessage = contextMessage;
        this.scanner = scanner;
    }

    public String getContextMessage() {
        return this.contextMessage;
    }

    public double ask() {
        return READ_NUMBER_FUNCTION.apply(menuMessage, scanner);
    }

}

With this in mind it will lead to following code at the caller:

... System.out.println(askForFirstNumber.getContextMessage()); Double d = askForFirstNumber.ask(); ...

The goal should be to have all console output statements in one compilation unit to be sure that you are not violating the single responsibility principle.

HistoryHolder

You provide a presentation method. You should separate presentation from datastructure.

Suggestions

1. Try to separate input from processing from ouput
2. Try to separate the datastructures from their representations
3. Try to introduce proper abstractions (console ouput, history)
4. Try to avoid unfavourable polymorphism that will offer concrete type information in the abstraction (Action enum)