# Simple beginner Java calculator

This is my first calculator in Java. I just started learning a week ago so any feedback (positive and negative) is welcome. Tell me what's good and what I can improve on.

import java.util.Scanner;

public class Calculator {

public static void main(String[] args) {

// Creates scanner object
Scanner scanner = new Scanner(System.in);

// Controls the start and stop of the program
boolean run = true;

// Sets and resets the operand and answer variables
int fnum = 0;
int snum = 0;

// While loop that keeps the program running until user exits
while (run) {

// Calculator interface
System.out.println("=====================================");
System.out.println("           Calculator V1.0         ");
System.out.println("=====================================");
System.out.println("2. Subtraction");
System.out.println("3. Multiplication");
System.out.println("4. Division");
System.out.println("5. Exit");

// Prompts user to choose an operation
int operation = scanner.nextInt();

// Does stuff depending on what integer the user enters
switch (operation) {
case 1:
System.out.printf("Enter first number: ");
fnum = scanner.nextInt();
System.out.printf("Enter second number: ");
snum = scanner.nextInt();
break;

case 2:
System.out.printf("Enter first number: ");
fnum = scanner.nextInt();
System.out.printf("Enter second number: ");
snum = scanner.nextInt();
break;

case 3:
System.out.printf("Enter first number: ");
fnum= scanner.nextInt();
System.out.printf("Enter second number: ");
snum = scanner.nextInt();
break;

case 4:
System.out.printf("Enter first number: ");
fnum = scanner.nextInt();
System.out.printf("Enter second number: ");
snum = scanner.nextInt();
break;

case 5:
System.out.println("Thank you for using the calculator!");
scanner.close();
run = false;
break;

default:
System.out.println("Invalid operation!");
break;
}
}
}
}


// Does stuff depending on what integer the user enters


That is not a good comment. A comment should say why the code does something if it is not clear and the code should say what it does (if the code is unclear, you can use a comment, but this is also a signal you should write clearer code), this comment is just plain noise.

If you are using the comment to remember what the code does, not just because some teacher/book said to, a comment here is perfectly fine:

// Choose action based on input


## Duplicated code

case 1:
System.out.printf("Enter first number: ");
fnum = scanner.nextInt();
System.out.printf("Enter second number: ");
snum = scanner.nextInt();
break;

case 2:
System.out.printf("Enter first number: ");
fnum = scanner.nextInt();
System.out.printf("Enter second number: ");
snum = scanner.nextInt();
break;


Anything catch your eye there? Lots and lots of copy/pasted code. Most of that can be moved to before or after the switch:

System.out.printf("Enter first number: ");
fnum = scanner.nextInt();
System.out.printf("Enter second number: ");
snum = scanner.nextInt();

switch (operation) {
case 1:
break;

case 2:
break;
}



## Output

This really is a nitpick for now, but this:

System.out.println("           Calculator V1.0         ");


Will display the same as this:

System.out.println("           Calculator V1.0");


## Methods

This is a pretty straightforward program, but you should start separating unique components of your program and creating more methods. For example, you could create a method for input, a method for calculating the answer, and a method for output.

## Enums

Another thing you should look into is using an enum to designate the selected operation. Using case Operation.Addition: is clearer than case 1:, and there is a very limited set of data that allowed, which is demonstrated and enforced by the enum, which only accepts the allowed set, and is not enforced by an int.

## Bug

Your division operation has a bug:

snum = scanner.nextInt();


What happens when the user enters 0? Your program will crash. You must always test for division by 0 in cases like this:

snum = scanner.nextInt();
if (snum == 0) {
System.out.println("Invalid input - cannot divide by 0.");
break;
}


Since this is a beginner program you can be excused for the very bad comments, but you still need to hear about them.

    // Creates scanner object
Scanner scanner = new Scanner(System.in);

// Controls the start and stop of the program
boolean run = true;

// Sets and resets the operand and answer variables
int fnum = 0;
int snum = 0;

// While loop that keeps the program running until user exits


Are all pretty much useless. Your comments should typically explain Why you have done something, and almost never explain What your code is doing.

It is completely obvious that a new scanner object is being created, and that the while loop will keep the program running until the user exits.

On the other hand, the comment that you have for fnum, snum, and answer is somewhat helpful. Unfortunately it is helpful for the wrong reasons.

int fnum = 0;
int snum = 0;


You should make a strong effort to make your variable names as descriptive as possible. In this case I would recommend simply calling the variables firstNumber and secondNumber. Using abbreviations like this is almost never worth the characters that you will save.

StringBuilder

Repeatedly calling System.out.println is very resource intensive. It doesn't matter very much for this program, but it could matter a lot when you are working on something more complicated later on. On the other hand, StringBuilder is very efficient.

So this:

System.out.println("=====================================");
System.out.println("           Calculator V1.0         ");
System.out.println("=====================================");
System.out.println("2. Subtraction");
System.out.println("3. Multiplication");
System.out.println("4. Division");
System.out.println("5. Exit");


Could become this:

StringBuilder stringBuilder = new StringBuilder();
stringBuilder.append("=====================================");
stringBuilder.append("\n");
stringBuilder.append("           Calculator V1.0         ");
stringBuilder.append("\n");
stringBuilder.append("=====================================");
stringBuilder.append("\n");
System.out.println(stringBuilder.toString());


It's more lines of code but it is much more performant.

EDIT As noted in the comments, in this case it is better to use a constant instead of StringBuilder. However if you are ever building a string inside of a loop, you will definitely want to be familiar with StringBuilder because in that case it really is much better.

As for the constant, you have a couple of options. At the very least you could make this a constant rather than typing it out twice:

String DIVIDING_LINE = "=====================================";


Note that constants in Java are typically declared in all caps. Or you could make the whole thing a constant like this:

    String MENU = "=====================================\n"
+ "           Calculator V1.0\n"
+ "=====================================\n"
+ "2. Subtraction\n";


That way if you have to create the menu again, you don't have to build the whole string again, you can just use the constant.

• I completely disagree on the usage of StringBuilder in this case. It's better to print to System.out directly. Remember that StringBuilder costs memory. Jul 28, 2015 at 23:45
• @SimonAndréForsberg The StringBuilder should be much faster than multiple println() calls. However, there is no point in using a StringBuilder to build a constant string. Just write System.out.print("=====\n" + "    Calculator V1.0    \n" + "=====\n" + "1. Addition\n" + …); — there will be zero runtime overhead since it will all be concatenated into one string constant at compile time. Jul 29, 2015 at 9:30
• @200_success I believe the performance difference is negligible in this case, and hurts readability more than it does good. Just my opinion though. Jul 29, 2015 at 11:02
        // Controls the start and stop of the program
boolean run = true;

// Sets and resets the operand and answer variables
int fnum = 0;
int snum = 0;

// While loop that keeps the program running until user exits
while (run) {


You can do without the run variable if you change the break in the switch to a return.

The comment before the int variables is currently incorrect, but we can make it correct by moving them into the loop.

        // While loop that keeps the program running until user exits
while (true) {
// Sets and resets the operand and answer variables
int first = 0;
int second = 0;


I'm not convinced that you need num in the name. You can always see that they are numbers by looking at the int in the declaration. The f and s aren't that clear about what they are intended to represent, so I'd expand them. Another possibility would be firstOperand and secondOperand. Operand is a more precise name than number and indicates that .

Consider defining at least one method to shorten the code in main.

Incorporating all suggestions (including some I like from other answers):

public class Calculator {
StringBuilder builder = new StringBuilder();

builder.append("=====================================\n");
builder.append("           Calculator V1.0\n");
builder.append("=====================================\n");
builder.append("2. Subtraction\n");
builder.append("3. Multiplication\n");
builder.append("4. Division\n");
builder.append("5. Exit\n");

return builder.toString();
}

public static void displayAnswer(int operation, int first, int second) {
switch (operation) {
case 1:
break;
case 2:
break;
case 3:
break;
case 4:
if (0 == second) {
System.out.println("Invalid input - cannot divide by 0.");
return;
}
break;
default:
System.out.println("Invalid operation!");
return;
}

}

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

while (true) {

int operation = scanner.nextInt();

if (5 == operation) {
System.out.println("Thank you for using the calculator!");
scanner.close();
return;
}

System.out.print("Enter first number: ");
int first = scanner.nextInt();
System.out.print("Enter second number: ");
int second = scanner.nextInt();

}
}
}


I'm not sure one way or the other on whether to use StringBuilder over System.out.println. If you do use it, save the string so you only need to build it once.

You don't need to put a break; in the last case of the switch statement. It doesn't hurt anything to have it, but it is unnecessary. Personally, I normally include it for a named case and leave it off for a default case. I feel that this increases the difference between default: and other cases.

You can use the labeled continue to avoid other special handling in the invalid input case.

It's possible to argue that it would be better to split displayAnswer into two methods where one calculates the answer and the other handles the display. That would complicate the logic here. It would help more if the calculator had buttons, etc. that needed to be handled. Because then you wouldn't be trying to handle operations, an exit signal, and invalid input all in the same block of code.

• this if (5 == operation) { is an unnecessary remnent from C, which can only seem weird to most java developers. Jul 29, 2015 at 19:14

There's some answers above dealing with optimisations that you should only need to care much further down the line (that's not saying that there is some truth to their comments).

I think the most important thing I would change about your code is the use of a Class structure, to define a calculator Object. This is an object you can call methods on to perform the actions, much in the same way as a traditional calculator. This has many benefits, but for now it's just "object oriented" design. Currently your Calculator class is just there to hold a "main()" method.

Note too the use of BigDecimal as a basis for the calculation. A "float" or "double" would also work to a point, but because of the way a double is stored, it can't accurately store certain values (0.1 for example). One thing to note with BigDecimal is the that class is immutable, that is once it has been created with a value, it will always contain this value (the same as the String class). This means you need to assign the result of an operation to a variable. See the "add" method below.

    public class Calculator {
private BigDecimal currentValue;

public Calculator() {
clearAll();
}

public void clearAll() {
this.currentValue = BigDecimal.ZERO;
}

public BigDecimal currentValue() {
return this.currentValue();
}

}

// these methods I'll let you implement
public void subtract(BigDecimal value)
public void multiply(BigDecimal value)
public void divide(BigDecimal value)
}


To use the Calculator in your "main" method, you would create a new instance of the Calculator, as a simple example:

public static void main(String[] args) {

Calculator calculator = new Calculator();

String entry = "123.999";



To print a BigDecimal out in the console as a String, you may need to use a DecimalFormat to print it in normal (not scientific) notation.

...
BigDecimal result = calculator.getCurrentValue();

// DecimalFormat with 10 decimal places
DecimalFormat outputFormat = new DecimalFormat("#0.##########");