# Evaluating a math expression given in string format

Posted on

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 {

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) {

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
}
``````

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].