Simple Java calculator with two operands

Question

I am taking a lynda dot com course online to start off. The course asks you to write a simple program:

1. Ask the user to input 2 values
2. Ask the user to input an operation
3. Use the operation on the 2 values and print the result

This was very different from the solution given by the course. Could anyone take a quick look at it and tell me if there's something I'm doing fundamentally wrong or inefficient? My suspicion is that I'm using too much memory.

package com.example.java;

import java.util.Scanner;

public class Main {

    public static void main(String[] args) {

        Scanner scanner = new Scanner(System.in);

        System.out.print("Enter a numeric value: ");
        String input1 = scanner.nextLine();
        System.out.print("Enter a numeric value: ");
        String input2 = scanner.nextLine();

        double double1 = Double.parseDouble(input1);
        double double2 = Double.parseDouble(input2);

        System.out.print("Choose and operation (+ - * /): ");
        String input3 = scanner.nextLine();

        double resultAdd = double1 + double2;
        double resultSub = double1 - double2;
        double resultMul = double1 * double2;
        double resultDiv = double1 / double2;

        switch (input3) {
            case "+":
                System.out.println("The answer is " + resultAdd);
                break;
            case "-":
                System.out.println("The answer is " + resultSub);
                break;
            case "*":
                System.out.println("The answer is " + resultMul);
                break;
            case "/":
                System.out.println("The answer is " + resultDiv);
                break;
        }
    }
}

Here is what the instructor wrote, for the record:

package com.example.java;

import java.util.Scanner;

public class Calculator2 {

    public static void main(String[] args) {
        String s1 = getInput("Enter a numeric value: ");
        String s2 = getInput("Enter a numeric value: ");
        String op = getInput("Choose an operation (+ - * /):");

        double result = 0;

        try {
            switch (op) {
                case "+":
                    result = addValues(s1, s2);
                    break;
                case "-":
                    result = subtractValues(s1, s2);
                    break;
                case "*":
                    result = multiplyValues(s1, s2);
                    break;
                case "/":
                    result = divideValues(s1, s2);
                    break;
                default:
                    System.out.println("Unrecognized operation!");
                    return;
            }

            System.out.println("The answer is " + result);

        } catch (Exception e) {
            System.out.println("Number formatting exception " + e.getMessage());

        }
    }

    private static double addValues(String s1, String s2) {
        double d1 = Double.parseDouble(s1);
        double d2 = Double.parseDouble(s2);
        return d1 + d2;
    }

    private static double subtractValues(String s1, String s2) {
        double d1 = Double.parseDouble(s1);
        double d2 = Double.parseDouble(s2);
        return d1 - d2;
    }

    private static double multiplyValues(String s1, String s2) {
        double d1 = Double.parseDouble(s1);
        double d2 = Double.parseDouble(s2);
        return d1 * d2;
    }

    private static double divideValues(String s1, String s2) {
        double d1 = Double.parseDouble(s1);
        double d2 = Double.parseDouble(s2);
        return d1 / d2;
    }

    private static String getInput(String prompt) {
        System.out.print(prompt);
        Scanner sc = new Scanner(System.in);
        return sc.nextLine();
    }

}

Answer 1

Your code

Simple is nice. But it would have been good to decompose your solution to multiple methods, for example at least:

• a method to read a double
• a method to perform the calculation: take the numbers and an operator as parameter and return the result

Other issues:

• Don't pre-calculate the result of all possible calculations when you will only use one of them
• The print statements are repetitive. If you decomposed the solution to multiple methods, naturally you would have factored out duplicated elements.
• Naming, as @Caridorc already covered.

The instructor's code

Sort of nicely written, decomposed, but this is bad code:

• Delaying the validation of input by carrying around the String values of supposed numeric values is a terrible idea. As @Caridorc said it too, the numeric input should have been read into the desired types (double in this example) and lift the burden of uncertainty from the rest of the code.

• The getInput method is a good idea, but poorly executed. Creating a new Scanner for every piece of input is a waste. Just like there is one System.in, a single Scanner could be used and passed to the method. The method is a huge missed opportunity for actually validating the input, and returning the desired type, instead of a String.

• Large try blocks are also better to avoid, and the large try block in the middle is just another consequence of delaying the input validation, that could have been done much better earlier.

Answer 2

Reduce the repetition of Double.parseDouble

The instructor repeats Double.parseDouble so many times... He should just parse the doubles as soon as he gets them to avoid such repetition.

Your code avoids such repetition and is simpler.

Error handling

I may note however that your code crashes on invalid inputs, while the instructors' handles them gently. If that was not required, however, I would have just kept the code simple like yours as doing more than what is requested does not give bonus points, on the reverse it shows that you cannot follow instructions precisely.

Naming

Both you and the instructor chose not-so-good names. He chose s1 and s2 and you chose input1 input2 input3. Such names do not convey the meaning of the value under them. In fact input1 and input2 may be avoided by parsing the input to double as soon as it is read. input3 may be renamed to operator to allow a fast understanding of its purpose. Very short abbreviations like op (used by the instructor) should also be avoided.

Overengeneering

Many times overenginnering a trivial task is not good, and while the instructor probably wants to teach you about having different methods and separation of concerns (good intentions), he chose a too easy example, nullyfying the benefit of modularity.

Answer 3

In addition to the excellent points covered in @Caridorc's and @janos's answer, it will be helpful to know about the hasNextDouble() and nextDouble() methods of the Scanner class that helps you pre-validate the user input.

Borrowing the instructor's approach of having a getDouble() method, coupled with a healthy dose of try-with-resources on the Scanner instance for efficient handling of the underlying I/O resource, we will have something like this:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        double firstNumber = getDouble(scanner, "Enter a number: ");
        double secondNumber = getDouble(scanner, "Enter a number: ");
        // ...
    }
}

private static double getDouble(Scanner scanner, String prompt) {
    System.out.println(prompt);
    while (!scanner.hasNextDouble()) {
        System.out.println("Invalid input, please try again.");
        scanner.next();
    }
    return scanner.nextDouble();
}

This might be getting ahead of what you are learning now, but what getDouble() is doing is to check if the next input "can be interpreted as a double value using the nextDouble() method". If not, it re-prompts the user, else the result of nextDouble() is returned from the method.

Answer 4

I see some problems with your code: It is not DRY, you calculate results that you never use, and you do "too much" in the switch-cases. Also, try to find expressive names for your variables (not based on the type, but on the function).

I know you're just a beginner, but maybe this is a good opportunity to learn about lambdas. There is an interface for the kind of "operation" you need here, which is called BinaryOperator. You can easily create instances by using lambdas, pass these instances around, and call them later. This split between creating some kind of operation, and actually using (or reusing) it is an incredible powerful tool. Have a look:

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

public class Main {

    private static Scanner scanner = new Scanner(System.in);

    public static void main(String[] args) {

        double operand1 = readNumber();
        double operand2 = readNumber();

        System.out.print("Choose an operation (+ - * /): ");
        BinaryOperator<Double> fn = getOperator(scanner.next());

        System.out.println("The answer is " + fn.apply(operand1, operand2));
    }

    private static BinaryOperator<Double> getOperator(String input) {
        switch (input) {
            case "+": return (x, y) -> x + y;
            case "-": return (x, y) -> x - y;
            case "*": return (x, y) -> x * y;
            case "/": return (x, y) -> x / y;
            default:
                throw new InputMismatchException("Unknown operator '" + input + "'");
        }
    }

    private static Double readNumber() {
        System.out.print("Enter a numeric value: ");
        return scanner.nextDouble();
    }
}

Of course, you could add some error handling, e.g. ask again when the user misspelled an operator. Also keep in mind that division by zero results in "Infinity", which is fine here, but may be a problem in other contexts.

Answer 5

Your solution will crash with a division-by-zero error if someone tries to calculate 4+0, whereas the instructors version will work correctly in that case because it doesn't try to divide unless its asked to.