Simple Calculator - revised

Question

This is my revised code of the Calculator:

import java.math.*;
import java.util.*;
public class Calculator {
 public static void main(String[] args) {
 Scanner sc = new Scanner(System.in);
 System.out.println("Welcome to the Calculator!");
 do {
 System.out.println("\nType a calculation:");
 String s = sc.nextLine();
 try {
 System.out.println("\nThe answer is: " + calculate(s));
 } catch (IllegalArgumentException e) {
 System.out.println("Oops! Illegal calculation.");
 }
 } while (doAgain(sc));
 System.out.println("\nThank you for using the calculator!");
 sc.close();
 }
 private static boolean doAgain(Scanner sc) {
 while (true) {
 System.out.println("\nAgain?");
 String s = sc.nextLine();
 if (s.equalsIgnoreCase("y") || s.equalsIgnoreCase("yes")
 || s.equalsIgnoreCase("t") || s.equalsIgnoreCase("true"))
 return true;
 else if (s.equalsIgnoreCase("n") || s.equalsIgnoreCase("no")
 || s.equalsIgnoreCase("f") || s.equalsIgnoreCase("false"))
 return false;
 }
 }
 public static Double calculate(String s) throws IllegalArgumentException {
 IllegalArgumentException exception = new IllegalArgumentException(
 "Illegal Calculation.");
 ArrayList<Double> nums = new ArrayList<Double>(s.length());
 ArrayList<String> op = new ArrayList<String>(s.length());
 getNumsAndOp(nums, op, s, exception);
 for (int i = 0; i <= nums.size(); i++) {
 if (op.contains("^")) {
 if (nums.get(op.indexOf("^") + 1).toString().contains(".")
 && !nums.get(op.indexOf("^")).toString()
 .matches("[0-9]+\\.0"))
 throw exception;
 nums.set(op.lastIndexOf("^"), Expressions.power(
 nums.get(op.lastIndexOf("^")),
 (int) ((nums.get(op.lastIndexOf("^") + 1)) + 0)));
 nums.remove(op.lastIndexOf("^") + 1);
 op.remove("^");
 } else if (op.contains("!")) {
 if (nums.get(op.indexOf("!")).toString().contains(".")
 && !nums.get(op.indexOf("!")).toString()
 .matches("[0-9]+\\.0"))
 throw exception;
 nums.set(op.indexOf("!"), (double) Expressions
 .factorial((int) (nums.get(op.indexOf("!")) + 0)));
 nums.remove(op.indexOf("!") + 1);
 op.remove("!");
 } else if (op.contains("/")) {
 multiplyOrDivide(nums, op, "/");
 } else if (op.contains("*")) {
 multiplyOrDivide(nums, op, "*");
 } else if (op.contains("-")) {
 plusOrMinus(nums, op, "-");
 } else if (op.contains("+")) {
 plusOrMinus(nums, op, "+");
 } else if (!op.isEmpty()) {
 throw exception;
 }
 }
 return nums.get(0);
 }
 private static void plusOrMinus(ArrayList<Double> nums,
 ArrayList<String> op, String plusOrMinus) {
 int index = op.indexOf(plusOrMinus);
 if (plusOrMinus.equals("+")) {
 nums.set(index, nums.get(index) + nums.get(index + 1));
 } else {
 nums.set(index, nums.get(index) - nums.get(index + 1));
 }
 nums.remove(index + 1);
 op.remove(index);
 }
 private static void multiplyOrDivide(ArrayList<Double> nums,
 ArrayList<String> op, String multiplyOrDivide) {
 int index = op.indexOf(multiplyOrDivide);
 BigDecimal bd = new BigDecimal(nums.get(index).toString());
 BigDecimal bd2 = new BigDecimal(nums.get(index + 1).toString());
 if (multiplyOrDivide.equals("*")) {
 bd = bd.multiply(bd2);
 bd.setScale(bd.scale() + bd2.scale());
 } else {
 bd = bd.divide(bd2);
 }
 nums.set(index, bd.doubleValue());
 nums.remove(index + 1);
 op.remove(index);
 }
 private static void getNumsAndOp(ArrayList<Double> nums,
 ArrayList<String> op, String s, IllegalArgumentException exception)
 throws IllegalArgumentException {
 String toAdd = "";
 double num = 0.0;
 double previousNum = 0.0;
 int iInARow = 0;
 int sInARow = 0;
 boolean hasDecimal = false;
 for (int i = 0; i < s.length(); i++) {
 previousNum = num;
 try {
 num = Integer.parseInt(s.charAt(i) + "");
 iInARow++;
 sInARow = 0;
 String dec = hasDecimal ? "." : "";
 hasDecimal = false;
 if (iInARow >= 2) {
 Double dou = previousNum;
 if (previousNum - dou.longValue() == 0)
 num = Double.parseDouble((int) previousNum + dec
 + (int) num);
 else
 num = Double.parseDouble(previousNum + dec + (int) num);
 }
 dec = "";
 try {
 Integer.parseInt(Character.toString(s.charAt(i + 1)));
 } catch (Exception e) {
 Character c;
 try {
 c = s.charAt(i + 1);
 } catch (StringIndexOutOfBoundsException exc) {
 c = 'a';
 }
 if (!c.equals('.'))
 nums.add(num);
 }
 } catch (Exception e) {
 Character c = s.charAt(i);
 if (c.equals('.')) {
 hasDecimal = true;
 continue;
 } else if (c.equals('!')) {
 nums.add(0.0);
 } else if (c.toString().equals(" ")) {
 continue;
 } else {
 sInARow++;
 iInARow = 0;
 }
 if (sInARow == 2) {
 throw exception;
 } else {
 toAdd = c.toString();
 }
 op.add(toAdd);
 if (toAdd.equals("!")) {
 try {
 s.charAt(i + 1);
 } catch (StringIndexOutOfBoundsException exc) {
 break;
 }
 }
 }
 }
 nums.trimToSize();
 op.trimToSize();
 }
}

• jsanc623: Why do you have combined functions (ie multiplyOrDivide) rather than having one function for multiplication and one for division? Yes, it's DRY, but its not clean.
• Roddy of the Frozen Peas: There exists a System.err.println function that should be leveraged in place of System.out.println for the IllegalArgumentException. Of course, an external logging library would be better still.

Answer 1

From your previous question:

I have a simple calculator program that needs some reviewing. Is this as efficient as it can get, or is there a better way?

I'll try tackling both processing speed and memory use.

---

Efficiency issues

Creating new strings often. Examples are on lines 116, 124, 127, 131, 149, 158. Use String.substring when you do need a part of your original string.

Testing boundaries with IndexOutOfBoundsException at 135 and 163. A quick profiling run with VisualVM had your code spending about 80% of its time throwing and catching exceptions in valid expressions. Do an explicit boundary check like i < s.length() instead.

Wrapping/unwrapping objects at 122, 135, 137, 139, 143, 144, 147. Consider using the == and != operators when comparing chars.

ArrayList.trimToSize() shrinks the underlying array's length to the number of elements, and is only useful for very long-lived lists or those you removed many elements from.

Repeated string parsing and pattern matching at 43 and 53. Using a more structured parsing approach would alleviate this.

Double → BigDecimal → Double mutating at multiplyOrDivide. There is little gain in doing this; consider keeping your numbers either as BigDecimals or as Doubles all the way.

---

Overall Code Quality

The items listed under efficiency issues are also code smells, so I won't repeat them here.

The exception re-use in line 35 (and thrown on 46, 56, and 70) is evil. It contaminates your method signature; it removes context and gives a misleading stack trace; it forces users of the method to create an exception instance regardless of whether or not it'll be actually thrown. If the input is illegal, throw an exception on the spot like so:

throw new IllegalArgumentException("Number expected, found: " + s);

---

Tips for improvement

Consider explicit lexical analysis. Try separating the string parsing from the calculation:

1. Parse strings into tokens. Don't look at operator syntax yet: just slice into numbers and operators.
2. Build a precedence tree. Here you check syntax and perhaps illegal operations (division by zero).
3. Actual evaluation.

This will allow you to handle more or different operators, such as parentheses, negation, and so on.

Look into the Scanner class for some helpful functions, like nextBigDecimal().

Premature optimisalisation is the root of evil. First get it working, then profile your application to catch efficiency problems, and then see what you can do about any issues that ping on the radar.

Aim for bite-sized functions. getNumsAndOp is a bit of a beast. multiplyOrDivide and plusOrMinus feel just about right in size. This will also make profiling easier for both you and the HotSpot compiler.