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#
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 char
s.
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#
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#
Tips for improvement
Consider explicit lexical analysis. Try separating the string parsing from the calculation:
- Parse strings into tokens. Don't look at operator syntax yet: just slice into numbers and operators.
- Build a precedence tree. Here you check syntax and perhaps illegal operations (division by zero).
- 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.