Problem
I am currently working on building a calculator using Java and JavaFX to practice my coding skills. I wrote the following methods to evaluate a given expression.
What can I improve? Is there anything you would recommend me to change to optimize my program?
public static String evaluate(String expression) {
char[] tokens = expression.toCharArray();
List<String> list = new ArrayList<>();
String s = "";
String operator = "";
String firstNum = "";
String secondNum = "";
boolean operationOnQueue = false;
for (int i = 0; i < tokens.length; i++) {
if (Character.isDigit(tokens[i])) {
s += Character.toString(tokens[i]);
} else {
list.add(s);
list.add(Character.toString(tokens[i]));
if (operationOnQueue) {
operationOnQueue = false;
secondNum = s;
list.set(list.lastIndexOf(firstNum), eval(firstNum, operator, secondNum));
list.remove(list.lastIndexOf(operator));
list.remove(list.lastIndexOf(secondNum));
}
if (tokens[i] == '*' || tokens[i] == '/') {
operationOnQueue = true;
operator = Character.toString(tokens[i]);
firstNum = list.get(list.lastIndexOf(operator) - 1);
}
s = "";
}
if (i == tokens.length - 1 && s.length() > 0) {
list.add(s);
if (list.get(list.size() - 2).equals("*") || list.get(list.size() - 2).equals("/")) {
firstNum = list.get(list.size() - 3);
operator = list.get(list.size() - 2);
secondNum = list.get(list.size() - 1);
list.set(list.size() - 3, eval(firstNum, operator, secondNum));
list.remove(list.size() - 2);
list.remove(list.size() - 1);
}
}
}
while (list.size() > 1) {
firstNum = list.get(0);
operator = list.get(1);
secondNum = list.get(2);
list.set(0, eval(firstNum, operator, secondNum));
list.remove(2);
list.remove(1);
}
return list.get(0);
}
public static String eval(String a, String operator, String b) {
double r = 0;
switch (operator) {
case "/":
r += Double.parseDouble(a) / Double.parseDouble(b);
break;
case "*":
r += Double.parseDouble(a) * Double.parseDouble(b);
break;
case "-":
r += Double.parseDouble(a) - Double.parseDouble(b);
break;
case "+":
r += Double.parseDouble(a) + Double.parseDouble(b);
break;
}
return Double.toString(r);
}
These are the results of my tests. The first line is the string inputted, along with the answer to the expression. The second line is the output of the program.
8+4/2+10*3+5*10 = 90
90.0
60*60+8+384/7/4-44*26 = 2477.71
2477.714285714286
64/16*18-51*7+5 = -280
-280.0
64/16*18-5100*7+5 = -35623
-35623.0
7325+92+44/4+57-84 = 7401
7401.0
14+54/9+1 = 21
21.0
7*4/7+9+12*3 = 49
49.0
Solution
How does it work?
Seems to be working now. What I can’t understand is how 🙂
Code should be readable and easily understandable by humans as well. You might even be one of the humans that have to work with code that you wrote a year or more ago..
There is a lot going on in the code that I cannot tell why it’s there and what its use is.
For example:
if (operationOnQueue) {
operationOnQueue = false;
secondNum = s;
list.set(list.lastIndexOf(firstNum), eval(firstNum, operator, secondNum));
list.remove(list.lastIndexOf(operator));
list.remove(list.lastIndexOf(secondNum));
}
I can read the code and see what it will do, but why? I have no clue.
So either try to explain the why in comments, or re-write the code to be more self-explanatory.
(Btw your code looks a bit like https://en.wikipedia.org/wiki/Shunting-yard_algorithm.)
Try to break big methods into smaller ones
You evaluate does a few things. The while loop in the end seems like a good candidate for a separate method. The second if in the for loop as well.
Don’t repeat yourself
There are a few Character.toString(tokens[i]) in the code. It’s easier to read if you extract that to a variable, for example String currentToken = Character.toString(tokens[i])
Handling error cases
What if the input is not a correct expression?
For example: 14+54/9+
Exception in thread "main" java.lang.IndexOutOfBoundsException: Index: 2, Size: 2
at java.util.ArrayList.rangeCheck(ArrayList.java:657)
at java.util.ArrayList.get(ArrayList.java:433)
at Math.evaluate(Math.java:64)
at Math.main(Math.java:96)
BTW: for more interesting links / implementations see here:
https://stackoverflow.com/a/114601/461499
There is one big issue in your setup, which reduces the usability, readability and quality of your code.
Single Responsibility Principle
Your function does both (1) parsing and evaluation, and (2) printing results.
public static String evaluate(String expression)
{
// .. parse expression
// .. evaluate the expression tree
// .. print the result as string
}
You have even managed to combine the evaluation and printing.
public static String eval(String a, String operator, String b)
{
// evaluate operation
// format the result to string
}
You should refactor your code.
Parsing and evaluation:
public static Double evaluate(String expression)
{
// .. parse expression
// .. evaluate the expression tree
}
public static Double eval(Double a, Operators operator, Double b)
{
// .. evaluate operation
}
public enum Operators { Add, Subtract, Multiply, Divide }
Printing
public static String print(Double value)
{
// .. print value
}
And there’s that infamous division by zero amongst other possible edge cases you should think about:
r += Double.parseDouble(a) / Double.parseDouble(b);
To make your code short, understandable, elegant, maintainable, and fast, I recommend to:
- implement separately syntax and semantics
- for syntax part, construct recursive descent parser.
- read something on theory, starting from Bacus-Naur form
- learn examples on Github recursive descent parser
- ask questions at Stackoverflow. Tag them with [recursive-descent].