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

Question

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");
       getNumbers();
       answer = firstNumber + secondNumber;
       System.out.println("This is the sum of the two numbers:  " + answer);
       calculator();
    }

    public static void multiplication() {
        System.out.println("Multiplication");
        getNumbers();
        answer = firstNumber * secondNumber;
        System.out.println("This is the product of the two numbers  " + answer);
        calculator();
    }

    public static void division() {
        System.out.println("Division");
        getNumbers();
        answer = firstNumber / secondNumber;
        System.out.println("This is the quotient of the two numbers:    " + answer);
        calculator();
    }

}

Answer 1

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.

Answer 2

Structure: Fields

firstNumber, secondNumber, and answer should not be fields. Fields are supposed to hold the state of an object across operations. But in your case, they are not actually used outside of the functions. And they shouldn't be either.

The result of an addition has nothing to do with the result or input of a previous multiplication, so they should be variables of those functions.

Instead of setting fields, your getNumbers function should just return the numbers (and make it singular, which makes it easier to use and also to reuse).

Structure: Functions

Ideally, functions should do one thing. Your functions do quite a bit more: They calculate, but they also print, get input, and resume calculator operations. For such a small project it may not matter that much, but generally, such a structure severely impacts testabiliy and reusability.

You should move getNumbers and calculator into the main calculator function. This also removes some duplication and thus makes your code shorter. Then, your calculation functions should just return the result, which can then also be printed in the main calculator function.

Naming

Objects are things, while functions are actions, and this should be reflected in their names. For example, instead of a functions called subtraction, you would call it subtract, because it isn't a subtraction, but it subtracts something. Same with calculator which may be startCalculator or calculate.

pick is also a bit generic. Sure, the user picked something, but what did they pick? chosenOperation might be clearer.

Misc

• don't import *, import the specific classes that you need to increase readability.
• your formatting is a bit off (indentation, too much vertical whitespace)
• your fields should generally be private if there is no good reason to make them public

Answer 3

Most tems have already been covered so I'll only give 1 other observation. You keep calling calculator instead of allowing the functions to return. .. eventually your program would crash with a "stack overflow" error. (OK it will take a while from human input but it's bad practice).

I would consider a while loop (repeat the prompt until they hit a key to exit or what not).

Answer 4

If I may ask, exactly how does your program end. I dont see any exit point for this code. I ran it and it kept asking me the four choices.

Also if you are only performing simple arithmetic operations, you can do so with a much lesser code than this.

There is a lot of duplicate code. For eg. You are getting the numbers each time for each operation. Why not just get them once from main and then work on them?

I've written my version that pretty much attains the same. I've also commented some thoughts between your existing code and mine. I've kept your input fields but made them private. Other stuff is explained in code

import java.util.Scanner;
//import java.math.*;//you have not used this anywhere.

    public class Calc{ // opening the bracket on the same line saves linespace, but it's a matter of opinion.

        static Scanner in = new Scanner(System.in);// Scanner is final, no need to declare it as final. 
        //public static double answer;//not needed as you are operating on new values every time.
        private double firstNumber;//In your code this is static. Why static when you are obtaining it for every operation??
        private double secondNumber; //why is it static in your code??

        public Calc(){// initializes the two numbers.

            getNumbers();

        }

        public void getNumbers(){

            System.out.println("First Number:");
            firstNumber=in.nextInt();
            System.out.println("Second Number:");
            secondNumber=in.nextInt();

        }

        public static void main(String... a) {

            System.out.println("Welcome to Calculator");
            System.out.println("Enter your Choice:");
            System.out.println(" 1.Add\n 2.Sub\n 3.Mul\n 4.Div\n 5.Exit\n\n");

            Calc c=null;
            int ch= in.nextInt();
            if(ch<5)
            c=new Calc();
            switch(ch){
/* // Since you are only implementing 4 simple operations, you can do it here itself.
   // When you add more though, I recommend using functions like you have done.*/

                case 1:
                    System.out.println(c.firstNumber+c.secondNumber);
                    break;
                case 2:
                    System.out.println(c.firstNumber-c.secondNumber);
                    break;
                case 3:
                    System.out.println(c.firstNumber*c.secondNumber);
                    break;
                case 4:
                    System.out.println(c.firstNumber/c.secondNumber);
                    break;
                case 5:
                    System.exit(1);
                default:
                    System.out.println("Wrong Choice.Enter your Choice:");
                    System.out.println("1.Add\n 2.Sub\n 3.Mul\n 4.Div\n 5.Exit\n\n");
                    ch= in.nextInt();
                    break;

            }
            main();

        }

    }

It's still rough here and there. It would be better with more functionality as well. Hope this clears some stuff for you. Keep trying.