current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 7 down vote favorite

I was wondering if anyone has any suggestions to improve my code. 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.

Specifically I'd 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();
    }

}
share|improve this question
2  
This may be a copy and paste issue, but your code indentation is not consistent and it makes it hard to read. In main() I'd use a switch statement rather than multiple "if then else if" statements. – pacmaninbw 12 hours ago
    
Oh okay, thanks. One thing I forgot to ask in the main post was why I have to return something in the getNumbers method and can I return anything? The code wouldn't work without it. – Zoratu 12 hours ago
    
You had to return something in get numbers because you declared it as a double. If you don't want to return anything declare it as void. – pacmaninbw 12 hours ago
up vote 5 down vote accepted

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.

share|improve this answer
up vote 3 down vote

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
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.