patternjavaModerate
Very basic calculator using methods
Viewed 0 times
methodsusingverycalculatorbasic
Problem
I have made a basic calculator using methods. This is my first attempt at using methods and would like to see if I can improve on this as there is a lot of repeated code.
```
import java.util.Scanner;
public class Calculator {
public static void main(String[] args) {
Scanner kb = new Scanner(System.in);
System.out.println("Simple Calculator");
System.out.println("\nHere are your options:");
System.out.println("\n1. Addition");
System.out.println("2. Subtraction");
System.out.println("3. Division");
System.out.println("4. Multiplication");
System.out.print("\nWhat would you like to do?: ");
int choice = kb.nextInt();
System.out.println();
if (choice == 1){
addition();
}
else if (choice == 2){
subtraction();
}
else if (choice == 3){
division();
}
else if (choice == 4){
multiplication();
}
System.out.println();
kb.close();
}
public static void addition(){
int nOne, nTwo;
Scanner kb = new Scanner(System.in);
System.out.println("Addition");
System.out.print("\nFirst Number: ");
nOne = kb.nextInt();
System.out.print("\nSecond Number: ");
nTwo = kb.nextInt();
kb.close();
System.out.println("\nSum: " + nOne + " + " + nTwo + " = " + (nOne + nTwo));
}
public static void subtraction(){
int nOne, nTwo;
Scanner kb = new Scanner(System.in);
System.out.println("Subtraction");
System.out.print("\nFirst Number: ");
nOne = kb.nextInt();
System.out.print("\nSecond Number: ");
nTwo = kb.nextInt();
kb.close();
System.out.println("\nSum: " + nOne + " - " + nTwo + " = " + (nOne - nTwo));
}
public static void division(){
int nOne, nTwo;
Scanner kb = new Scanner(System.in);
```
import java.util.Scanner;
public class Calculator {
public static void main(String[] args) {
Scanner kb = new Scanner(System.in);
System.out.println("Simple Calculator");
System.out.println("\nHere are your options:");
System.out.println("\n1. Addition");
System.out.println("2. Subtraction");
System.out.println("3. Division");
System.out.println("4. Multiplication");
System.out.print("\nWhat would you like to do?: ");
int choice = kb.nextInt();
System.out.println();
if (choice == 1){
addition();
}
else if (choice == 2){
subtraction();
}
else if (choice == 3){
division();
}
else if (choice == 4){
multiplication();
}
System.out.println();
kb.close();
}
public static void addition(){
int nOne, nTwo;
Scanner kb = new Scanner(System.in);
System.out.println("Addition");
System.out.print("\nFirst Number: ");
nOne = kb.nextInt();
System.out.print("\nSecond Number: ");
nTwo = kb.nextInt();
kb.close();
System.out.println("\nSum: " + nOne + " + " + nTwo + " = " + (nOne + nTwo));
}
public static void subtraction(){
int nOne, nTwo;
Scanner kb = new Scanner(System.in);
System.out.println("Subtraction");
System.out.print("\nFirst Number: ");
nOne = kb.nextInt();
System.out.print("\nSecond Number: ");
nTwo = kb.nextInt();
kb.close();
System.out.println("\nSum: " + nOne + " - " + nTwo + " = " + (nOne - nTwo));
}
public static void division(){
int nOne, nTwo;
Scanner kb = new Scanner(System.in);
Solution
Nice job! I hope you find the comments below useful as you progress.
-
Redundancy: Any time you are copy-pasting code, alarm bells should go off in your head. You do this in multiple places. Most notable is:
What that means is that this is a prime candidate for encapsulation into its own method. Something like this:
Now we can simply call this method whenever we need to get numbers from the user. Much easier, and we've reduced the complexity of our code.
Another thing that's redundant is your constant re-declaration of
This is the basic rule for how constants are defined in Java. I named the variable
Some other small things
You should consider using a
... or you could write it all in-line, if it looks better to you, since each block only really does one thing:
This would also be a perfect use-case for enums, but that my be getting a bit too complicated for a starting project.
Small final note: I think you have a bug. The line where you display the results always says "sum", even when you're doing division, multiplication, or subtraction. :)
- Variable names: Try to have variable names which describe what the variable actually represents or holds. For example, I have no idea why you named your
Scanner"kb".
- Method names: Method names should always be verb phrases (actions) which describe what the method does. So for example, your
addition()method might be better namedperformAddition()or some such. Even justadd()might work, though that might be a bit misleading and lead someone reading your code to believe that all the method does is add two numbers, when really the core functionality is user interface.
-
Redundancy: Any time you are copy-pasting code, alarm bells should go off in your head. You do this in multiple places. Most notable is:
System.out.print("\nFirst Number: ");
nOne = kb.nextInt();
System.out.print("\nSecond Number: ");
nTwo = kb.nextInt();What that means is that this is a prime candidate for encapsulation into its own method. Something like this:
private static int[] getNumbers() {
int[] numbers = new int[2];
System.out.print("\nFirst Number: ")
numbers[0] = scanner.nextInt();
System.out.print("\nSecond Number: ");
numbers[1] = scanner.nextInt();
return numbers;
}Now we can simply call this method whenever we need to get numbers from the user. Much easier, and we've reduced the complexity of our code.
Another thing that's redundant is your constant re-declaration of
kb = new Scanner(System.in). You really only need to do this once. If it were me, I'd have it as a static field for the entire class, e.g.,private static final Scanner STDIN = new Scanner(System.in);This is the basic rule for how constants are defined in Java. I named the variable
STDIN because this is a convention for referring to the "standard input" stream, but SCANNER or something would be just as good.Some other small things
You should consider using a
switch statement on your choice variable:switch(choice) {
case 1:
performAddition();
break;
case 2:
performSubtraction();
break;
case 3:
performDivision();
break;
case 4:
performMultiplication();
break;
default:
System.out.println("Invalid choice!");
break;
}... or you could write it all in-line, if it looks better to you, since each block only really does one thing:
switch(choice) {
case 1: performAddition(); break;
case 2: performSubtraction(); break;
// etc.
}This would also be a perfect use-case for enums, but that my be getting a bit too complicated for a starting project.
Small final note: I think you have a bug. The line where you display the results always says "sum", even when you're doing division, multiplication, or subtraction. :)
Code Snippets
System.out.print("\nFirst Number: ");
nOne = kb.nextInt();
System.out.print("\nSecond Number: ");
nTwo = kb.nextInt();private static int[] getNumbers() {
int[] numbers = new int[2];
System.out.print("\nFirst Number: ")
numbers[0] = scanner.nextInt();
System.out.print("\nSecond Number: ");
numbers[1] = scanner.nextInt();
return numbers;
}private static final Scanner STDIN = new Scanner(System.in);switch(choice) {
case 1:
performAddition();
break;
case 2:
performSubtraction();
break;
case 3:
performDivision();
break;
case 4:
performMultiplication();
break;
default:
System.out.println("Invalid choice!");
break;
}switch(choice) {
case 1: performAddition(); break;
case 2: performSubtraction(); break;
// etc.
}Context
StackExchange Code Review Q#47026, answer score: 13
Revisions (0)
No revisions yet.