5
\$\begingroup\$

This Java code is reading integers and arithmetic operations from a string and evaluating the arithmetic operation.

For example:

For the input "32+54*27*4/13", output is 480.61538

For the input "9/4+2*3-1/2", output is 7.75

Where I need suggestion is:

Currently this code is including more than 60 lines and on the interview I was asked to write it in less than 40 lines .

How can I reduce code lines to below 40 lines?

import java.util.ArrayList;
public class Calculator {
    public static void main(String[] args) {
        // TODO Auto-generated method stub
        System.out.println(new Calculator().calculate("32+54*27*4/13"));//Should produce 480.61538

    }   
    public  double calculate(String expression){        
        ArrayList<String> operators=new ArrayList<String>();
        ArrayList<Double> operands=new ArrayList<Double>(); 
        int lastOperatorIndex=0; //Spot last operator position for extracting integers from string
        int priorityOperationCount=0; // Check how many * or / included in the expression
        //Start reading expression
        for(int i=0; i<expression.length();i++)
        {
            String currentChar=String.valueOf(expression.charAt(i));
            if(isOperator(currentChar) && i!=0){ //Check if its an operator
                operators.add(currentChar);
                if(isPriortyOperator(currentChar))//Check if its an priorty operator like * or /
                    priorityOperationCount++;
                //Extract the integers between two operators i.e; in 2+23/43 '23' should be extracted
                int startIndex=lastOperatorIndex==0?0:lastOperatorIndex+1;                      
                String operand=expression.substring(startIndex, i);
                operands.add(Double.parseDouble(operand));
                lastOperatorIndex=i;                
            }           
            else if(i==expression.length()-1){//If its the last integer on the expression
                String operand=lastOperatorIndex==0?(expression.substring(lastOperatorIndex)):(expression.substring(lastOperatorIndex+1));//Subtract the last operand
                operands.add(Double.parseDouble(operand));        
            }
        }
        //Start evaluating expression
        int i=0;
        while( !operators.isEmpty() ) //Loop through until there are no operators left on the expression
        {
           String currentOperator=operators.get(i);        
           double leftOperand=operands.get(i);
           double rightOperand=operands.get(i+1);           
            if( (currentOperator.equals("+") || currentOperator.equals("-"))  && (priorityOperationCount==0)  )//If current operator is + or - and there are no priority operators on the expression then simply evaluate the expression
            {   
                   operands.remove(i);
                   double result=currentOperator.equals("+")? leftOperand+rightOperand:leftOperand-rightOperand;
                   operands.set(i,result);
                   operators.remove(i);
            }                   
            else if(currentOperator.equals("*") || currentOperator.equals("/")  ) //If current operator is * or / evaluate the expression
            {
                   operands.remove(i);
                   double result=currentOperator.equals("*")?  leftOperand*rightOperand: leftOperand/rightOperand;
                   operands.set(i,result);
                   operators.remove(i);
                   priorityOperationCount--;
                   i--;
            }
            i=(operators.size()>0 && i>=operators.size()-1)  ||  ( priorityOperationCount==0 ) ? 0:i+1;//Go back to first operator if its the end of operators array or there are no priority operators left
        }       
        return operands.size()>0?operands.get(0):0; //Return 0 if there are no operands
    }  
    public  boolean isOperator(String chr){
        return (  ((chr.equals("+")) || (chr.equals("-")) || (chr.equals("*")) || (chr.equals("/"))));
    }
    public  boolean isPriortyOperator(String chr){
        return (  (chr.equals("*")) || (chr.equals("/")) );
    }   
}
\$\endgroup\$
1
  • 1
    \$\begingroup\$ IMHO the interviewers asked you the wrong question. This code is purely procedural. When applying for a job as Java programmer I'd expect an candidate to show a more object oriented approach. And That's what I would ask you: rewrite it applying OO principles. \$\endgroup\$ Commented Mar 9, 2017 at 8:39

1 Answer 1

4
\$\begingroup\$

Your code is mostly in one big headache-inducing function. Interviewers don't really want to try to understand complicated code like that. Your submission would likely be rejected at first glance unless you made some major improvements in strategy.

In particular, parsing the expression character-by-character is too tedious. You can use regular expressions for that.

Ideally, expression evaluation should be done using a recursive-descent parser and shunting-yard algorithm. But a 40-line solution calls for a more hackish solution. I'd split the expression into terms (breaking at + and - signs). Within each term, perform any multiplications and divisions. I'd verbally apologize to the interviewer that this is all a hack, and fails to generalize to support parentheses, unary plus/minus signs, and other complications. But it is good enough to handle limited cases consisting of just those four operations, such as those two given examples.

Also, unlike your submission, my solution below does reject some expressions as invalid.

import java.util.regex.*;

public class Calculator {
    private static final Pattern EXPR_RE = Pattern.compile("\\G\\s*([+-]?)\\s*([^+-]+)"),
                                 TERM_RE = Pattern.compile("\\G(^|(?<!^)\\*|(?<!^)/)\\s*(\\d*\\.?\\d+)\\s*");

    public static double calculate(String expr) {
        Matcher m = EXPR_RE.matcher(expr);
        double sum = 0;
        int matchEnd;
        for (matchEnd = -1; m.find(); matchEnd = m.end()) {
            sum += (("-".equals(m.group(1))) ? -1 : +1) * evalTerm(m.group(2));
        }
        if (matchEnd != expr.length()) {
            throw new IllegalArgumentException("Invalid expression \"" + expr + "\"");
        }
        return sum;
    }

    private static double evalTerm(String term) {
        Matcher m = TERM_RE.matcher(term);
        double product = Double.NaN;
        int matchEnd;
        for (matchEnd = -1; m.find(); matchEnd = m.end()) {
            switch (m.group(1)) {
                 case "*": product *= Double.parseDouble(m.group(2)); break;
                 case "/": product /= Double.parseDouble(m.group(2)); break;
                 case "":  product  = Double.parseDouble(m.group(2)); break;
            }
        }
        if (matchEnd != term.length()) {
            throw new IllegalArgumentException("Invalid term \"" + term + "\"");
        }
        return product;
    }

    public static void main(String[] args) {
        System.out.println(calculate(args[0]));
    }
}

This solution could be written more elegantly, but it fits in exactly 40 lines, as written above.

Note that the calculate() function is static, to avoid the pointless new Calculator() instantiation.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.