HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Very basic calculator using methods

Submitted by: @import:stackexchange-codereview··
0
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);

Solution

Nice job! I hope you find the comments below useful as you progress.

  • 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 named performAddition() or some such. Even just add() 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.