Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link
added 57 characters in body
Source Link
palacsint
  • 30.4k
  • 9
  • 82
  • 157
  1. Instead of doubles (and floating point numbers) consider using BigDecimals. Floating point numbers are not precise. Here is an example:

     Enter expression: 1.03-0.42
     Expression: 1.03-0.42
     Prepared expression: 1.03-0.42
     Solving expression: 1.03-0.42
     Solving expression: 0.6100000000000001
     Answer: 0.6100000000000001
    
  2. DIVIDERS currently mutable. You could wrap it with Collections.unmodifiableList to avoid accidental modification. A more compact form is Guava's ImmutableList:

     private static final List<Character> DIVIDERS = ImmutableList.of('*', '/', '-', '+');
    
  3. In some places I would use aString.contains instead of indexOf. It's more meaningful, closer to the English language, easier to read since you cat get rid of the comparison. In conditionals like this

     expression.indexOf("*") > 0 | expression.indexOf("/") > 0
    

(It might not fit well for trigonometric functions.)

- [Why not use Double or Float to represent currency?][1]
- *Effective Java, 2nd Edition, Item 48: Avoid float and double if exact answers are required*
  1. DIVIDERS currently mutable. You could wrap it with Collections.unmodifiableList to avoid accidental modification. A more compact form is Guava's ImmutableList:

     private static final List<Character> DIVIDERS = ImmutableList.of('*', '/', '-', '+');
    
  2. In some places I would use aString.contains instead of indexOf. It's more meaningful, closer to the English language, easier to read since you cat get rid of the comparison. In conditionals like this

     expression.indexOf("*") > 0 | expression.indexOf("/") > 0
    
  1. Instead of doubles (and floating point numbers) consider using BigDecimals. Floating point numbers are not precise. Here is an example:

     Enter expression: 1.03-0.42
     Expression: 1.03-0.42
     Prepared expression: 1.03-0.42
     Solving expression: 1.03-0.42
     Solving expression: 0.6100000000000001
     Answer: 0.6100000000000001
    
  2. DIVIDERS currently mutable. You could wrap it with Collections.unmodifiableList to avoid accidental modification. A more compact form is Guava's ImmutableList:

     private static final List<Character> DIVIDERS = ImmutableList.of('*', '/', '-', '+');
    
  3. In some places I would use aString.contains instead of indexOf. It's more meaningful, closer to the English language, easier to read since you cat get rid of the comparison. In conditionals like this

     expression.indexOf("*") > 0 | expression.indexOf("/") > 0
    
  1. Instead of doubles (and floating point numbers) consider using BigDecimals. Floating point numbers are not precise. Here is an example:

     Enter expression: 1.03-0.42
     Expression: 1.03-0.42
     Prepared expression: 1.03-0.42
     Solving expression: 1.03-0.42
     Solving expression: 0.6100000000000001
     Answer: 0.6100000000000001
    

(It might not fit well for trigonometric functions.)

- [Why not use Double or Float to represent currency?][1]
- *Effective Java, 2nd Edition, Item 48: Avoid float and double if exact answers are required*
  1. DIVIDERS currently mutable. You could wrap it with Collections.unmodifiableList to avoid accidental modification. A more compact form is Guava's ImmutableList:

     private static final List<Character> DIVIDERS = ImmutableList.of('*', '/', '-', '+');
    
  2. In some places I would use aString.contains instead of indexOf. It's more meaningful, closer to the English language, easier to read since you cat get rid of the comparison. In conditionals like this

     expression.indexOf("*") > 0 | expression.indexOf("/") > 0
    
Source Link
palacsint
  • 30.4k
  • 9
  • 82
  • 157

I agree with that a different approach would be better, so just a few random notes, mostly about the current code:

  1. It's good to know that it's already in the JDK, in form a script engine. So, if you don't want to reinvent the wheel just use it:

     import javax.script.ScriptEngine;
     import javax.script.ScriptEngineManager;
    
     final ScriptEngineManager engineManager = new ScriptEngineManager();
     final ScriptEngine engine = engineManager.getEngineByName("JavaScript");
     String expression = "11 + (Math.exp(2.010635 + Math.sin(Math.PI/2)*3) + 50) / 2";
     System.out.println(engine.eval(expression)); // prints 110.99997794278411
    

The syntax is not the same (you need the Math. prefix) but I guess it can be changed.

  1. Instead of doubles (and floating point numbers) consider using BigDecimals. Floating point numbers are not precise. Here is an example:

     Enter expression: 1.03-0.42
     Expression: 1.03-0.42
     Prepared expression: 1.03-0.42
     Solving expression: 1.03-0.42
     Solving expression: 0.6100000000000001
     Answer: 0.6100000000000001
    
  2. DIVIDERS currently mutable. You could wrap it with Collections.unmodifiableList to avoid accidental modification. A more compact form is Guava's ImmutableList:

     private static final List<Character> DIVIDERS = ImmutableList.of('*', '/', '-', '+');
    
  3. In some places I would use aString.contains instead of indexOf. It's more meaningful, closer to the English language, easier to read since you cat get rid of the comparison. In conditionals like this

     expression.indexOf("*") > 0 | expression.indexOf("/") > 0
    

I'd use the even more compact

    StringUtils.containsAny(expression, "*/")

(from Apache Commons Lang)

  1. The following lines also could be replaced with StringUtils:

     int multPos = expression.indexOf("*");
     int divPos = expression.indexOf("/");
    
     pos = Math.min(multPos, divPos);
     if (multPos < 0)
         pos = divPos;
     else if (divPos < 0)
         pos = multPos;
    

Use indexOfAny:

    pos = StringUtils.indexOfAny(expression, "*/")

It's the same.

  1. Instead of returning with an error string, like

     return "Failure!";
    

I think you should throw an exception and stop the processing immediately, there is no point to "continue" the processing with wrong subresult (or to pretend that it continues). (The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

The same is true for the default branch of calcShortExpr. If you call it with an invalid "divider" it's definitely an error in the program and it should stop.

  1. The divider variable actually contains the operator which is a little bit confusing. I'd rename it for better readability.