A simple command-line calculator

Question

I created a simple command line calculator, and then after some reading I made a few tweaks and then remade it with a friends idea to make it more simpler. I'm wondering if this can be more optimized or if any part of the code can be done "better"?

I recently started learning Java and wanted for this first project of mine to end up the "best possible" written, so I can start learning on what is a "good code".

One of the things that was changed over time was the use of the Scanner.

• Firstly I had it create a new one on each iteration of kalkulator(); but that was obviously a unnecessary waste.
• Then I had a static scanner static Scanner scanMe = new Scanner(System.in); but somewhere I read It was better to pass it as an argument, because something that doesn't belong anywhere doesn't have place in a good code unless it is really necessary.

Here is what I ended up with so far:

"Main" Method:

public class SimpleCalculator {


public static void main(String[] args) {

    Scanner scanMe = new Scanner(System.in);

    kalkulator(scanMe);
}

"Helper" Methods:

static double scanDouble(Scanner scan){
    while (!scan.hasNextDouble()){

        System.out.println("Invalid number! Please try again.");
        scan.nextLine();
    }

    return scan.nextDouble();
}

static String scanOperator(Scanner scan){
    String In = scan.next();

    while (!(In.equals("+") || In.equals("-") || In.equals("*") || In.equals("/") || In.equals("end"))) {

        System.out.println("Invalid operator! Please select either: +,-,*,/");
        In = scan.next();
    }

    return In;
}

The Main Method:

    public static void kalkulator(Scanner scan) {

    double prviB = 0, drugiB = 0, rezultat = 0;
    String operator;


    System.out.println("Enter the 1st number: ");
    prviB = scanDouble(scan);

    System.out.println("Enter the 2nd number: ");
    drugiB = scanDouble(scan);

    System.out.println("Select an operator: (+,-,*,/) , or type 'end' for termination: ");
    operator = scanOperator(scan);


    switch (operator) {
    case "+":
        rezultat = prviB + drugiB;
        break;

    case "-":
        rezultat = prviB - drugiB;
        break;

    case "*":
        rezultat = prviB * drugiB;
        break;

    case "/":
        rezultat = prviB / drugiB;
        break;

    case "end":
        System.out.println("Terminated.");
        scan.close();
        System.exit(0);
        break;

    default: 
        break;
    }

    System.out.println("Result: " + rezultat);
    System.out.println("");

    kalkulator(scan);
}

Answer 1

try-with-resources

You can wrap your Scanner instance using try-with-resources for safe and efficient handling of the underlying I/O resource:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        kalkulator(scanner);
    }
}

Program flow

public static void kalkulator(Scanner scanner) {
    // ...
    System.out.println("Result: " + rezultat);
    System.out.println("");
    kalkulator(scan);
}

This is the wrong approach to get your method to loop, by having it call itself recursively. At a sufficiently large number, your program will eventually crash as it can no longer reliably track the nested method calls. You should use a simple loop mechanism outside of the method, so that kalkulator() can return quickly and normally for its next invocation by the loop. For example:

public static void main(String[] args) {
    try (Scanner scanner = new Scanner(System.in)) {
        do {
            kalkulator(scanner);
            System.out.println("Do you want to end? Type end if so.");
        } while (!scanner.next().equals("end"));
    }
}

// changed to private modifier
private static void kalkulator(Scanner scanner) {
    // ...
    System.out.println("Result: " + rezultat);
    System.out.println(); // empty "" not required
}

Handling logic

While I think it's helpful that the other answers are pointing you towards an enum-driven approach, for a beginner, I feel starting with basic String comparison is... ok.

I wouldn't change much here, but I'll suggest renaming your String In variable in scanOperator(Scanner) to something like input to stick to the camelCase convention you have followed elsewhere.

On a related note to the previous section, since you shouldn't be checking for the end as an operator here, you can safely drop the check.

// added private modifier, renamed to getOperator
private static String getOperator(Scanner scanner) {
    boolean isOperator = false;
    String input;
    do {
        System.out.println("Type an operator: (+,-,*,/)");
        input = scanner.next();
        isOperator = input.equals("+") || input.equals("-")
                        || input.equals("*") || input.equals("/");
        if (!isOperator) {
            System.out.println("Invalid operator.");
        }
    } while (!isOperator);
    return input;
}

private static void kalkulator(Scanner scanner) {
    double first = getNumber(scanner, "1st");  // renamed from scanDouble, second
    double second = getNumber(scanner, "2nd"); // argument used for printing the prompt
    String input = getOperator(scanner);
    double result;
    switch (input) {
        case "+":
            result = first + second;
            break;
        case "-":
            // ...
        case "*":
            // ...
        case "/":
            // ...
        default:
            break;
    }
    System.out.println("Result: " + result + "\n");
}

Language

It's better to stick to English for all your class/method/variable names too, instead of a mix. This is to promote better understanding of your code by others who may not understand your native written language.

Answer 2

Nice improvement is to use Enum with behaviour. This is an example from Effective Java book :

public enum Operation {
    PLUS("+") {
        double apply(double x, double y) {
            return x + y;
        }
    },
    MINUS("-") {
        double apply(double x, double y) {
            return x - y;
        }
    },
    TIMES("*") {
        double apply(double x, double y) {
            return x * y;
        }
    },
    DIVIDE("/") {
        double apply(double x, double y) {
            return x / y;
        }
    };
    private final String symbol;

    Operation(String symbol) {
        this.symbol = symbol;
    }

    @Override
    public String toString() {
        return symbol;
    }

    abstract double apply(double x, double y);
}

Answer 3

The organization of the code looks ok so far.

• The naming of methods and variables should be consistent in one language (preferential in english)
• If you change the method scanOperator to support more operators and forget to adjust the switch statement, the output is "Result: 0" (which is wrong).

To fix the last point, you could handle the default state by throwing an exception or something like that, or better you could create an operator class that contains the whole logic for an operator.

Answer 4

Scanner as an argument is not necessary here.

I would suggest, in order, that you

1. Implement something that works on simple binary operations.
2. Implement something to parse out those operations from string form.
3. Add in dialogue to interface with the user.
4. Take in command line arguments.

A Simple example that illustrates all 4 of the above:

First an operator enumeration.

public enum Operator {
    ADD('+'),
    SUBTRACT('-'),
    MULTIPLY('*'),
    DIVIDE('/'),
    MOD('%'),
    EXPONENTIATE('^');

    private final char symbol;

    Operator(char symbol) {
        this.symbol = symbol;
    }

    public char symbol() {
        return symbol;
    }

    @Override
    public String toString() {
        return name().charAt(0) + name().substring(1).toLowerCase();
    }
}

For now, you can think of an enumeration as a useful way to store related series of values that won't change. A calculator is a good opportunity to look into them rather than referencing the values directly, and if you're curious there's an interesting way we can apply them to the purpose of a calculator.

Then the calculator program itself.

import java.util.Scanner;

public class Calculator{
    public static void main(String[] args) {
        if (args.length == 3) {
            System.out.println(calculate(args));
        } else {
            Scanner input = new Scanner(System.in);
            System.out.print("Enter a binary calculation: ");
            System.out.println(calculate(input.nextLine().split("\\s+")));
        }
    }

    private static double calculate(String[] args) {
        if (args.length != 3) {
            throw new IllegalArgumentException("Can only calculate binary operations");
        }

        double input1 = Double.parseDouble(args[0]);
        char operator = args[1].charAt(0);
        double input2 = Double.parseDouble(args[2]);

        if (input2 == 0 && (operator == Operator.DIVIDE.symbol() || operator == Operator.MOD.symbol())) {
            throw new IllegalArgumentException("Cannot divide by 0");
        }

        return calculation(input1, operator, input2);
    }

    private static double calculation(double input1, char operator, double input2) {
        double result = 0;
        switch(operator) {
            case '+':
                result = input1 + input2;
            break;
            case '-':
                result = input1 - input2;
            break;
            case '*':
                result = input1 * input2;
            break;
            case '/':
                result = input1 / input2;
            break;
            case '%':
                result = input1 % input2;
            break;
            case '^':
                result = Math.pow(input1, input2);
            break;
        }
        return result;
    }
}

Notice that I optionally directly accept argument which is something desirable for a CLI. This example has a lot you can still improve on, including input validation and a more direct utilization of the enumeration among other things.

If you're unfamiliar with the split function I'm simply converting whatever the user gives us to a String array, with the indexes separated by spaces (what the regex "\s++" means). so I may treat it as if it was invoked from the command line. If you want to learn more about Regex, you can check out other patterns or use HackerRank's decent introduction/practice platform here.