Refactoring calculator expression

Question

I was doing a simple exercise to calculate the amount based on an expression.

For example:

(((2*2)+(3*5/5)+(2*5))) outputs 17

((2*2)(3*5/5)(2*5)) output 120 etc

I have written the code, but I am not satisfied with what I've done. Could you please help me to review the code and suggest the best approach for that?

import java.util.ArrayList;
public class Calculator {
 private static ArrayList<String> data;
 private static Element element = new Element();
 int startIndex = 0;
 int endIndex = 0;
 public int length = 0;
 boolean startFlag = false;
 public double calculatedValue = 0;
 public Calculator() {
 // TODO Auto-generated constructor stub
 }
 public static void main(String[] args) {
 // String testExpression = "(((2*2)+(3*5/5)+(2*5)))";//17
 String testExpression = "((2*2)(3*5/5)(2*5))"; // 120--
 new Calculator().parsedExpression(testExpression);
 }
 public void parsedExpression(String testExpression) {
 System.out.println(testExpression + " Before formatting");
 testExpression = testExpression.replaceAll("\\)\\(", ")*("); // To
 // change
 // (a+b)(b+c)
 // to
 // (a+b)*(b+c)
 System.out.println(testExpression + " After first formatting");
 data = CalculatorUtil.parseExpression(testExpression);
 System.out.println("Total Sum====" + process());
 }
 public double process() {
 System.out.println("Array:ist" + data);
 startIndex = 0;
 endIndex = 0;
 length = data.size();
 System.out.println("===" + data.indexOf("("));
 if (data.indexOf("(") >= 0) {
 getCalIndex();
 System.out.println("startIndex=" + startIndex + "==" + endIndex);
 }
 if (startIndex < endIndex) {
 double value = calculate(startIndex, endIndex);
 calculatedValue = value;
 System.out.println("CalculatedValue=" + value);
 // Reset arrayList
 if (startIndex >= 0 || endIndex <= length) {
 // Resize the list
 ArrayList<String> tempList = new ArrayList<>();
 int j = 0;
 for (int i = 0; i < startIndex - 1; i++) {
 tempList.add(data.get(i));
 }
 tempList.add(value + "");
 for (int i = endIndex; i < length; i++) {
 tempList.add(data.get(i));
 }
 data = tempList;
 process();
 }
 }
 return calculatedValue;
 }
 private void getCalIndex() {
 endIndex = 0;
 startIndex = 0;
 for (String element : data) {
 endIndex++;
 startIndex++;
 if (element.equals(")")) {
 for (int i = endIndex - 1; i >= 0; i--) {
 if (data.get(i).equals("(")) {
 return;
 }
 startIndex--;
 }
 }
 }
 }
 public double calculate(int startIndex, int endIndex) {
 int j = 0;
 element = new Element();
 for (int i = startIndex; i < endIndex; i++) {
 switch (data.get(i)) {
 case "*" :
 addOrManipulateEntitySecond('*', i, j);
 break;
 case "/" :
 addOrManipulateEntitySecond('/', i, j);
 break;
 case "-" :
 addOrManipulateEntityFirst('-', i, j);
 break;
 case "+" :
 addOrManipulateEntityFirst('+', i, j);
 break;
 default :
 break;
 }
 }
 /** Printing the Elements **/
 double operateCalculate = operate(element.getFirst(),
 element.getOperator(), element.getSecond());
 System.out.println("OperateCacluate=" + startIndex + "=" + endIndex
 + "=" + operateCalculate);
 return operateCalculate;
 }
 private static void addOrManipulateEntityFirst(char o, int i, int j) {
 if (element.getOperator() == '\u0000') {
 element = new Element(Double.valueOf(data.get(i - 1)),
 Double.valueOf(data.get(i + 1)), o);
 } else {
 element.setFirst(element.operate());
 element.setSecond(Double.valueOf(data.get(i + 1)));
 element.setOperator(o);
 }
 }
 private void addOrManipulateEntitySecond(char o, int i, int j) {
 if (element.getOperator() == '\u0000') {
 element = new Element(Double.valueOf(data.get(i - 1)),
 Double.valueOf(data.get(i + 1)), o);
 } else {
 element.setSecond(operate(element.getSecond(), o,
 Double.valueOf(data.get(i + 1))));
 // elements[j].setFirst(Double.valueOf(data.get(i+1)));
 // elements[j].setOperator(o);
 }
 }
 private double operate(double first, char o, double second) {
 double result = 0;
 switch (o) {
 case '*' :
 result = first * second;
 break;
 case '/' :
 result = first / second;
 break;
 case '+' :
 result = first + second;
 break;
 case '-' :
 result = first - second;
 break;
 default :
 break;
 }
 if (result == 0) {
 result = calculatedValue;
 }
 return result;
 }
}
public class Element {
 private double first;
 private double second;
 private char operator;
 private double result;
 public Element() {
 }
 public Element(double first, double second, char operator) {
 this.first = first;
 this.second = second;
 this.operator = operator;
 System.out.println(this);
 }
 public double operate() {
 switch (operator) {
 case '*' :
 result = first * second;
 break;
 case '/' :
 result = first / second;
 break;
 case '+' :
 result = first + second;
 break;
 default :
 break;
 }
 return result;
 }
 @Override
 public String toString() {
 // TODO Auto-generated method stub
 return "[ " + first + " " + operator + " " + second + " ]";
 }
 public double getFirst() {
 return first;
 }
 public void setFirst(double first) {
 System.out.println("First : " + first);
 this.first = first;
 }
 public double getSecond() {
 return second;
 }
 public void setSecond(double second) {
 this.second = second;
 }
 public char getOperator() {
 return operator;
 }
 public void setOperator(char operator) {
 this.operator = operator;
 }
 public double getResult() {
 return result;
 }
 public void setResult(double result) {
 this.result = result;
 }
}
import java.util.ArrayList;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
public class CalculatorUtil {
 public static final String INTEGER_FROM_STRING = "\\d+";
 public static final String FLOAT_FROM_STRING = "(\\d+[.]\\d+)";
 public static final String OPERATORS_FROM_STRING = "(\\/)|(\\-)|(\\+)|(\\*)|(\\%)|(\\()|(\\))|(\\^)";
 public static final String NUMBERS_FROM_STRING = FLOAT_FROM_STRING+"|"+INTEGER_FROM_STRING;
 public static final String NUMBERS_AND_OPERATORS_FROM_STRING = NUMBERS_FROM_STRING+"|"+OPERATORS_FROM_STRING;
 public static final String MULTIPLICATION_COMMON = "\\)\\(";
 public static final String MULTIPLICATION_VALID = ")*(";
 private static Pattern pattern;
 private static Matcher matcher;
 public CalculatorUtil() {
 // TODO Auto-generated constructor stub
 }
 /**
 * 
 * @param input
 * @return The data as ArrayList<String>
 */
 public static ArrayList<String> parseExpression(String input)
 {
 pattern = Pattern.compile(NUMBERS_AND_OPERATORS_FROM_STRING);
 matcher = pattern.matcher(input);
 ArrayList<String> data = new ArrayList<String>();
 while(matcher.find()) data.add(matcher.group());
 return data;
 }
}

• amon: Do you have any experience with writing parsers? There are less convoluted ways to do what you're doing, such as writing a bog-standard recursive descent parser.
• Teresa Carrigan: Have you considered converting the input to Reverse Polish Notation? Not having the user do it, just taking the input, convert it to RPN, and then evaluate the RPN. This would allow the user to skip unneeded parentheses.
• Alexei Kaigorodov: do google search for "recursive descent parser for arithmetic expressions".

Answer 1

I agree with the point in the comments. The approach you can implement that would simplify your code heavily is a recursion. The structure would be following the idea that you have interface/class Element, lets say, as you have it here but that would be just an interface. This interface would have implementord for binary and unary and 0-nary operation. Binary op would have childs plus,minus and so on, unary would have childs parentheses and unary minus and 0-nary's child is a number. Number has zero arguments so hence it can be considered 0-nary operation. The childs of those classes would be of type Element.

The interface would implement the method calculate and if you want to print it out also a method print(outputstream).

That is regarding how to parse. You would definitely like to use recursion because it is easier and shorter to write and also the case you are solving is of a recursive character.

I liked how you used matching for checking if it a valid input. That is always good to validate your inputs. With the recursive approach you may validate it on the go, not all at the beginning.