6
\$\begingroup\$

I am a beginner in Java programming, and I created a calculator as my first Java project. Therefore, can you review my code and provide feedbacks/ tips?

import java.util.Scanner;

public class Calculator {
    public static void main(String[] args) {

        double firstnum, secondnum, answer1, answer2, answer3, answer4, answer5, answer6; 
        String text;
        int loop = 0;

        Scanner input = new Scanner (System.in);

        while (loop == 0) {

        System.out.println("Enter your first number: ");  
        firstnum = input. nextDouble();

        System.out.println("Please enter the operation you would like to execute: ");
        text = input.next(); 

        System.out.println("Enter your second number: ");
        secondnum = input.nextDouble(); 

        answer1= firstnum+secondnum;
        answer2= firstnum-secondnum;
        answer3= firstnum*secondnum;
        answer4= firstnum/secondnum;
        answer5= (100*(firstnum/secondnum));
        answer6= Math.pow(firstnum,secondnum);

        switch (text) {

        case "+" :
            System.out.println(answer1);
            break; 

        case "-" :  
            System.out.println(answer2);
            break; 

        case "*" : 
            System.out.println(answer3);
            break; 

        case "x" : 
            System.out.println(answer3);
            break; 

        case "/" :  
            System.out.println(answer4);
            break; 

        case "%" :  
            System.out.println(answer5);
            break; 

        case "^" :  
            System.out.println(answer6);
            break; 

        default : 
            System.out.println("Your operation was not recongized.");

            }
        }
    }
}
\$\endgroup\$

6 Answers 6

4
\$\begingroup\$

I'm learning Java too!

Looking at this the only thing I might suggest is that you only do the calculations that you need to. You could then remove all answer variables and print the result of the calculations directly, saving time and memory, albeit not much.

In terms of presentation, I personally would use lowerCamelCase for variable naming, e.g. firstNum, and I would also indent all of the cases as they are within the switch block. Finally, for input. nextDouble(); I'd make sure there was no space there.

Despite what I say for the presentation, that part of it is totally up to you, so do what you prefer, as long as it looks clear!

\$\endgroup\$
4
\$\begingroup\$
  • Fix your indentation within the while loop and the switch.
  • You could just use a break to end the loop, or at least make loop a boolean instead.
  • Instead of having separate answer variables, you can just create an ArrayList of them:

    ArrayList<double> answers = new ArrayList<>(6);
    

    Not only can you avoid cluttering up the code with more of the same variables (if more of them will be added), but you can take advantage of ArrayList's features.

  • Try to put more of this into separate methods. main() should do as little as possible.
\$\endgroup\$
1
  • 1
    \$\begingroup\$ For what it's worth, that switch formatting is the default indentation for Eclipse IDE. \$\endgroup\$
    – corsiKa
    Commented Jul 24, 2016 at 4:35
1
\$\begingroup\$
  1. Why are you computing all the operations when only one is required?
  2. During division operation check for divide by zero?
  3. Double is a big enough data type. Can it overflow especially during multiplication?
  4. What happens if user enters a string instead of numbers?
  5. You can break it into small functions and return the results.

    class Calculator { public static void main(String[] args) {

        double firstnum, secondnum, answer1, answer2, answer3, answer4, answer5, answer6;
        String text;
        int loop = 0;
    
        Scanner input = new Scanner(System.in);
    
        while (loop == 0) {
    
            System.out.println("Enter your first number: ");
            if (input.hasNextDouble()) {
                firstnum = input.nextDouble();
            } else {
                // error or retry
            }
    
            System.out.println("Please enter the operation you would like to execute: ");
            text = input.next();
    
            System.out.println("Enter your second number: ");
            if (input.hasNextDouble()) {
                secondnum = input.nextDouble();
            } else {
                // error or retry for input
            }
    
            answer1 = firstnum + secondnum;
            answer2 = firstnum - secondnum;
            answer3 = firstnum * secondnum;
            answer6 = Math.pow(firstnum, secondnum);
            // before division check if secondnum != 0
            if (secondnum == 0) {
                // exception or some handling mechanism
            }
            answer4 = firstnum / secondnum;
            answer5 = (100 * (firstnum / secondnum));
    
            switch (text) {
    
            case "+":   
                System.out.println(answer1);
                break;
            case "-":
                System.out.println(answer2);
                break;
    
            case "*":
                System.out.println(answer3);
                break;
            case "x":
                System.out.println(answer3);
                break;
    
            case "/":
                System.out.println(answer4);
                break;
    
            case "%":
                System.out.println(answer5);
                break;
    
            case "^":
                System.out.println(answer6);
                break;
    
            default:
                System.out.println("Your operation was not recongized.");
    
            }
        }
    }
    

    }

\$\endgroup\$
1
\$\begingroup\$

In addition to what everyone else has already said (type checking, overflow checking, naming, indenting, etc.), if you don't mind calculating results even when you don't need them, you could easily skip the whole switch block with a Map:

    System.out.println("Enter your first number: ");  
    firstnum = input. nextDouble();

    System.out.println("Please enter the operation you would like to execute: ");
    String operation = input.next(); 

    System.out.println("Enter your second number: ");
    secondnum = input.nextDouble(); 

    Map<String, Double> results = new HashMap<>();
    results.put("+", firstnum + secondnum);
    results.put("-", firstnum - secondnum);
    results.put("*", firstnum * secondnum);
    results.put("x", firstnum * secondnum);
    results.put("/", firstnum / secondnum);
    results.put("%", 100*(firstnum + secondnum));
    results.put("^", Math.pow(firstnum, secondnum));

    if (results.get(operation) == null) {
        System.out.println("Your operation was not recongized.");
    } else {
        System.out.println(results.get(operation).toString());
    }
\$\endgroup\$
0
\$\begingroup\$

Why don't you perform calculations only when needed? This would save space allocation. You can do something like this:

    import java.util.Scanner;

public class Calculator {
  public static void main(String[] args) {

    double firstnum, secondnum, answer; 
    String text;
    int loop = 0;

    Scanner input = new Scanner (System.in);

    while (loop == 0) {

    System.out.println("Enter your first number: ");  
    firstnum = input. nextDouble();

    System.out.println("Please enter the operation you would like to execute: ");
    text = input.next(); 

    System.out.println("Enter your second number: ");
    secondnum = input.nextDouble(); 


    switch (text) {

    case "+" :
        answer = firstnum+secondnum;
        System.out.println(answer);
        break; 

    case "-" :
        answer = firstnum-secondnum;        
        System.out.println(answer);
        break; 

    case "*" :
        answer = firstnum*secondnum;
        System.out.println(answer);
        break; 

    case "/" : 
        answer = firstnum/secondnum); 
        System.out.println(answer);
        break; 

    case "%" :  
        answer = (100*(firstnum/secondnum));
        System.out.println(answer);
        break; 

    case "^" :  
        answer = Math.pow(firstnum,secondnum);
        System.out.println(answer);
        break; 

    default : 
        System.out.println("Your operation was not recongized.");

        }
    }
}
}
\$\endgroup\$
0
\$\begingroup\$

Instead of this:

int loop = 0;
while (loop == 0) {
    // Some code
}

you can write this:

while (true) {
    // Some code
}

Instead of this:

System.out.println("Please enter the operation you would like to execute: ");
text = input.next();

the variable should not be called text, but operator.

You could extract the calculation into a separate method:

static double calculate(double firstnum, String operator, double secondnum) {
    switch (operator) {
    case "+":   // Note how you can combine multiple cases.
    case "plus":
        return firstnum + secondnum;
    case "-":
    case "minus":
        return firstnum - secondnum;
    // … and so on …
    default:
        throw new IllegalArgumentException("Unknown operator: " + operator);
    }
}

and then use it like this:

try {
    double answer = calculate(firstnum, text, secondnum);
    System.out.println(answer);
} catch (IllegalArgumentException e) {
    System.out.println(e.getMessage());
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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