Basic calculator that takes 2 numbers and does an operation with them

The code works perfectly fine for me (compiled it in BlueJ and Eclipse) but I was wondering what other more experienced programmers thought of it. I'd specifically like to know if it could be shorter or be made more resource efficient.

```
/**
* This program is just a simple addition calculator that can use whole numbers or decimals.
*
* @author Christopher Goodburn
* @version 6/4/2016
*/
import java.util.Scanner;
import java.math.*;

public class Calculator
{
private static final Scanner askScanner = new Scanner(System.in);
public static double answer;
public static double firstNumber;
public static double secondNumber; //makes variables for the whole class

public static void main(String[] args) {
calculator();
}

public static void calculator() {

System.out.println("Basic calculator");
System.out.println("Pick one:");
System.out.println("1. Addition");
System.out.println("2. Subtraction");
System.out.println("3. Multiplication");
System.out.println("4. Division");
int pick = askScanner.nextInt();

if(pick == 1) {
addition();
}
else if(pick == 2) {
subtraction();
}
else if(pick == 3) {
multiplication();
}
else if(pick == 4) {
division();
}
else {
System.out.println("You need to choose 1, 2, 3, or 4");
calculator();
}
askScanner.close();
}

private static double getNumbers() {
System.out.println("First number?");
firstNumber = askScanner.nextDouble();
System.out.println("Second Number?");
secondNumber = askScanner.nextDouble();
return firstNumber;
}

public static void subtraction() {
System.out.println("Subtraction");
getNumbers();
answer = firstNumber - secondNumber;
System.out.println("This is the difference of the two numbers: " + answer);
calculator();
}

public static void addition() {
System.out.println("Addition");

The main issue with your code is repetition. The 4 methods calculating the result based on user input are sketched in the same way:

public static void operation() {
    // get numbers
    // perform operation and print result
    // return to main loop
}

As a result, there is copy-pasted code inside those 4 methods.

The first remark is that all of them will need to get numbers from the user. Instead of having each of addition, subtraction, etc. to get the numbers, retrieve them before hand from the user. In the same way, all of them invoke calculator(); to ask the user numbers again.

A first possiblity is therefore to have:

int pick = askScanner.nextInt();

getNumbers();
// perform operation based on input
calculator();

with a sample operation being:

public static void addition() {
    System.out.println("Addition");
    answer = firstNumber + secondNumber;
    System.out.println("This is the sum of the two numbers:  " + answer);
}

That said, this still leaves a design issue:

private static final Scanner askScanner = new Scanner(System.in);
public static double answer;
public static double firstNumber;
public static double secondNumber; //makes variables for the whole class

Having static global variables to keep the numbers input by the user isn't a good idea. What you want instead is to have local variables with the minimum possible scope.

You'll need to drop the method getNumbers() completely: it needs to have global variables (because it only returns one value and not the two). Also, the operation methods won't operate on the global variables anymore; instead, they will be given the two operands as parameter. And, lastly, they won't set the result global variable but return the result.

As such, the code would be:

if (pick == 1) {
    addition(firstNumber, secondNumber);
} else if (pick == 2) {
    subtraction(firstNumber, secondNumber);
} else if (pick == 3) {
    multiplication(firstNumber, secondNumber);
} else if (pick == 4) {
    division(firstNumber, secondNumber);
} else {
    System.out.println("You need to choose 1, 2, 3, or 4");
}

with a sample operation being:

public static double addition(double firstNumber, double secondNumber) {
    System.out.println("Addition");
    double answer = firstNumber + secondNumber;
    System.out.println("This is the sum of the two numbers:  " + answer);
    return answer;
}

You could also consider using a switch instead of the if/else blocks:

switch (pick) {
case 1: addition(firstNumber, secondNumber);
case 2: subtraction(firstNumber, secondNumber);
case 3: multiplication(firstNumber, secondNumber);
case 4: division(firstNumber, secondNumber);
default: System.out.println("You need to choose 1, 2, 3, or 4");
}

which makes the code a bit shorter.

As a final note: your code is an infinite loop right now, the user has no way to exit. It would be nice to introduce an option giving the user the possibility to quit the program.

StackExchange Code Review Q#130105, answer score: 11