0
\$\begingroup\$

I have asked a question on how to make a CLI calculator accept infinite numbers until you specify that you want the answer. Now I have figured out how to do it, but I do really want to know how can I improve my code.

package ribchinski;

import java.util.Scanner;

public class Calculator {
    public static double answer;
    public static Scanner in = new Scanner(System.in);
    public static boolean done = false;

    public static void main(String[] args) {

        System.out.println("Enter your expression:");
        System.out.println("Num: ");
        double firstNum = in.nextDouble();
        in.nextLine();

        while (!done) {
            System.out.println("Operator: ");
            String operand = in.nextLine();

            if (operand.equals("=")) {
                break;
            }
            System.out.println("Num: ");
            double secondNum = in.nextDouble();
            in.nextLine();

            calculate(firstNum, operand, secondNum);
            firstNum = answer;
        }
        System.out.println("Answer: " + answer);
    }

    public static void calculate(double num1, String equ, Double num2) {

        switch (equ) {

        case ("+"):
            answer = (num1 + num2);
            break;

        case ("-"):
            answer = (num1 - num2);
            break;

        case ("*"):
            answer = (num1 * num2);
            break;

        case ("/"):
            answer = (num1 / num2);
            break;
        }
    }
}

Any comments on this would be helpful!

\$\endgroup\$
3
\$\begingroup\$

Nice solution. This code is pretty legible, which is great if you have to debug anything. There are a few simple ways you can make your program more user-friendly and resistant to user error.

Input:

For the user, it can be tedious to type in their values on every line, always being asked for

Num:
Operator:
Num:
Operator:
   ...

Can you use in.nextDouble() and in.next(".") to read in all of your numbers and operators at once, from a line like 10+9-7 or 10+9-7=? in.next(String pattern) matches a regular expression, and . is the pattern for any single character.

Error Handling:

What happens if the user types in a bad operator? If someone tries to ask for 11+10_9, the operation is ignored in this case. Instead of getting the result of 11+10-9 = 12, they could get a result of 21 without realizing there was a mistake. A good way to protect against that is to use a default: statement in your switch block. A way to handle these errors and let the program or the user know there was a mistake is by writing

default: throw new IllegalArgumentException(equ + " is not a valid operator");

Object Oriented Programming:

Generally, static variables should be reserved for variables that are shared between multiple instances of a class, and any other variables should be made local or non-static. The static variables you've defined here work properly, but reorganizing the methods and changing the variables into local ones will make it easier to add functionality to this calculator later.

public static double answer;

Rather than saving the answer in a static or object-level variable, you can return it from your functions. This lets you keep more careful track of what makes changes to it. Imagine if you had a lot of different functions like calculate that incrementally update the answer every time they execute. If you started seeing the wrong answer due to a bug in the code, it could be difficult to trace it to the broken function.

To handle answer the object-oriented way, rename main() to whatever you like, change both of your functions from void to double type, and return the partial answer and the end answer from each of them, handling them locally. You can make a new main() that calls your renamed function and handle the printing of the answer in main() instead of the function that does the work.

public static boolean done = false;

done is never actually changed or used. while(true) is functionally equivalent to the loop that you wrote. This is a fairly common pattern, and there are a few ways to go about organizing it. Yours is handy because the variable done communicates that this loop will continue until something is 'done'. The best place to explain to the reader how a while() loop is supposed to terminate is right there in the condition.

One more descriptive (but also less clean) way to write that condition would be

String operand;
while((String operand = in.nextLine()) != '='){

This assigns the operator to operand and performs the comparison without the need for a break statement. You can use a do-while loop to handle the first number and print "Operator: " to the console before reading operand, if you like.

public static Scanner in = new Scanner(System.in);

This is often tolerated as a static variable, since we're very unlikely to change the fact that we're reading from System.in in the middle of our calculations. On the other hand, in doesn't need to be static, and since it's a resource that should be closed when we're done with it, the best practice is to make it a local variable or manage it carefully as needed. The object-oriented approach is to use a loosely-coupled function like readExpression() for your input, and then let all of the calculation happen in a different function.

You might also want to be consistent with your arguments in

public static void calculate(double num1, String equ, Double num2){

double is a primitive type, while Double is a class. That distinction won't cause a logical error here, but consistency always helps when a program grows larger and more complex.

Order of Operations:

Your program handles each step of the calculation correctly, but it doesn't understand Order of Operations. That's a more complex concept that you'll have to look at a whole expression for instead of being able to handle each operation piece by piece.

The fully object-oriented solution is to treat the expression and the individual operations as their own objects and apply logic to those objects piece by piece. You could, for instance, create a list of numbers and operations, and scan through that list, applying the highest-priority (multiplication and division in this case) operations first.

The fully functional solution is to scan the expression and use recursion to decide which numbers need to into calculate() first, returning the results of each calculate() step up until the base function returns the final answer.

There are many ways to do it, and you can think about how you'd like to implement it in your calculator

\$\endgroup\$
  • 1
    \$\begingroup\$ I understand that it would be far more user friendly if the program took the whole equation at once, but I am not that advanced enough. I've been changing the String to char, but that only gave me ERRORS and would not work at all. Some help would be appreciated. \$\endgroup\$ – ViceroyFaust Sep 18 '17 at 12:49
  • \$\begingroup\$ My bad. Looks like Scanner doesn't have a nextChar(). Here are some ways to read one character in from a Scanner: stackoverflow.com/questions/13942701/… Looks like in.next(".") is the way to go. I'll edit my answer. \$\endgroup\$ – Alex Sage Sep 18 '17 at 19:20
  • \$\begingroup\$ It looks like nextDouble also has some trouble if you have a line that contains more than just the number you're reading. stackoverflow.com/questions/10831224/… Is a fix for that problem. \$\endgroup\$ – Alex Sage Sep 18 '17 at 19:42

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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