Return to Answer

##Addendum: Comments on code logic

Addendum: Comments on code logic

Added: I've asked a question my self here on Code Review, which implements a calculator like the one I've described above (with exception of parentheses) written in Python. Hopefully this could still give you some hints on another approach for making a calculator.

Added: I've asked a question my self here on Code Review, which implements a calculator like the one I've described above (with exception of parentheses) written in Python. Hopefully this could still give you some hints on another approach for making a calculator.

Added: I've asked a question my self here on Code Review, which implements a calculator like the one I've described above (with exception of parentheses) written in Python. Hopefully this could still give you some hints on another approach for making a calculator.

##Addendum: Comments on code logic

In the following I'm going to ignore the restructure I've suggested, but try to give some comments on your existing code. This is to help you understand why you've been given comments that your code is a mess, having bad logic, and so on.

• Choose better and more meaningful names – The exp could be short of both expression and exponentiation, which is rather confusing when you only do exponentiation in your code. You also intermix exponents and power in containsExponents and loopPower, which is slightly confusing. I would rather use the full expression or possibly text, and containsPowerOperator and calculatePowers for these methods.

  Another example are the yesBeforenoAfter & co variables. These are both confusing and unused, and I'll come back to these later on.

  Lastly the operandOneStart and operandTwoEnd, I think I would rather use firstOperandStartIdx and secondOperandEndIdx which would clearer convey what they are used for.

• Extract more into functions - You could easily separate the entire loop for getting operands into a new function. You could call it like double firstOperand = new Operand(expression, firstOperandStartIdx), and if bold you let the firstOperand be a dedicated class having optional original text, value, and startIdx and endIdx parts.

• Simplify the rebuild of the expression – Instead of all of the code you've given with the yesBeforenoAfter & co, you could make this into a function as well, which simply concatenates the following string parts:

  • The substring from start of string to firstOperand.startIdx, or nothing if that is 0
  • The string version of the current result
  • The substring from secondOperand.endIdx to end of string, or nothing if that is 0

• Flag variables and first operand... – There are some logic related to keeping track of the start of the first operand and whether you've found the operator or not which I'm not entirely sure how it actually works. (And as previously suggested, this would be way simpler if you'd already processed the list into tokens like numbers and operators. :-).

  However I think you might have a bug here, and should test with expressions like 10^3 + 11^2 and see whether they give the expected output or not. I think I would have replaced the onFirst with setting the firstOperandStartIdx = -1 and testing against that. I would most likely also invert the if-statements so that it's clearer at the top what is happening, see refactored code below.

• Feature/bug: Your code will break on two exponential expression in the same expression – If I'm not mistaken your code will only accept one exponential expression, as you do a break after finding the operator. In my refactored code below, this is not handled either. But you should look into it

• Why do the containsExponents at all? – The code as it stands runs through the entire expression twice if it has the exponent operator. However you could simplify this by simply calling the loopPower (or calculatePowers:-) ) method once. The method would then loop the string once, and if nothing is found, it return the original expression. If the operator is found it still has only looped it once, but now it has rebuilt the expression without the exponential part, and are ready for next step.

• Simplify the isCharNumerical() – As suggested by πάντα ῥεῖ, you could simplify this function quite a bit...

Refactored code

I'll provide you with some untested code if you choose to proceed with your implementation and not going the route via tokens. This would need for a new class, Operand, something like the following:

public class Operand {
 String text;
 double value;
 int startIdx;
 int endIdx;
 public Operand(String text, int startIdx) {
 this.startIdx = startIdx;
 this.text = "";
 for (int idx = startIdx; idx < text.length(); idx++) {
 char currentChar = text.charAt(id);
 if (isCharNumerical(currentChar)) {
 this.text += currentChar;
 } else {
 this.endIx = idx - 1;
 break;
 }
 }
 this.value = Double.valueOf(this.text);
 }
}

And then your calculatePowers would look something like the following untested code:

public static String calculatePowers(String text) {
 int firstOperandStartIdx = -1;
 boolean foundOperator = false;
 int startNextPart = 0;
 for (int i = 0; i < text.length(); i++) {
 char currentCharacter = text.charAt(i);
 if (!isCharNumerical(currentCharacter)) {
 
 foundOperator = (currentCharacter == '^');
 
 // If this was not the operator we're looking for
 // flag that we need to start looking for a new operand
 if (!foundOperator) {
 firstOperandStartIdx = -1;
 }
 } else {
 // If start of operand hasn't been found, set this 
 // index to be start of number
 if (firstOperandStartIdx < 0) {
 firstOperandStartIdx = i;
 } 
 if (foundOperator) {
 Operand firstOperand = new Operand(text, firstOperandStartIdx);
 Operand secondOperand = new Operand(text, i);
 
 String new_expression = text.substring(startNextPart, firstOperand.startIdx);
 new_expression += String.valueOf(Math.pow(firstOperand.value, secondOperand.value);
 new_expression += text.substring(secondOperand.endIdx + 1, text.length());
 
 // This will need a little tweaking, if you want
 // the newly calculated part to be part of the next
 // expression as well... This is merely a start
 // related to fixing the only one exponential expression bug...
 startNextPart = secondOperand.endIdx;
 }
 }
 }
 return new_expression;
}