Evaluating expressions with various mathematical methods

Question

I have started to learn Java and I need your critique for my first program. It is an interpreter that can calculate any expression including abs(), sqrt(), min(), max(), round() and pow().

What can you say about this? Are there any code defects or pointless comments?

Main class

import java.io.*;
public class Main {
 static boolean Error=false;
 public static void main(String args[]) throws Exception{
 System.out.println("Type an expression to calculate \n" +
 "abs(x) function returns absolute value of x\n" +
 "sqrt(x) function returns square root of value x\n" +
 "min(x,y,...) function returns a minimum value between x,y,...\n" +
 "max(x,y,...) function returns a maximum value between x,y,...\n" +
 "round(x) function returns a round value of x\n" +
 "pow(x,y) function returns an exponentiation of x,y\n" +
 "Type \"exit\" to exit");
 while (true){
 BufferedReader read = new BufferedReader(new InputStreamReader(System.in));
 String stringForProcessing =read.readLine(); //Input a string to work with it
 String output= Calculating.calculating(stringForProcessing); //Calculating
 System.out.println(output);//Output
 Error=false;
 }
 }
}

Calculating class

import java.lang.Math;
import java.util.ArrayList;
public class Calculating {
 static String calculating(String workingString){
 ArrayList<String> separatedString = new ArrayList<>();
 int Position=0;
 boolean Negative=false;
 if(Main.Error){ //return error report
 return(workingString);
 }
 while(Position<workingString.length()){ //reducing spaces
 if(workingString.charAt(Position)==' '){
 while (((Position)<workingString.length())
 &&(workingString.charAt(Position)==' ')){
 Position++;
 }
 }
 else if (workingString.charAt(Position)=='-'&&(Position==0 //checking value for negativity
 || workingString.charAt(Position-1)=='-'
 || workingString.charAt(Position-1)=='+'
 || workingString.charAt(Position-1)=='*'
 || workingString.charAt(Position-1)=='/')){
 Negative=true;
 Position++;
 }
 else if ((workingString.charAt(Position)=='-' //checking for repeating operators
 || workingString.charAt(Position)=='+'
 || workingString.charAt(Position)=='*'
 || workingString.charAt(Position)=='/')
 &&(Position==0
 || workingString.charAt(Position-1)=='-'
 || workingString.charAt(Position-1)=='+'
 || workingString.charAt(Position-1)=='*'
 || workingString.charAt(Position-1)=='/')){
 Main.Error=true;return("Error: two or more operators in a row");
 }
 separatedString.add(Character.toString(workingString.charAt(Position)));
 if (workingString.charAt(Position)=='('){ //Creating bracer stack to parse
 int BrcrStack=1;
 String Temp="";
 while (((Position+1)<workingString.length())){
 switch (workingString.charAt(Position+1)){
 case '(':BrcrStack++;break;
 case ')':BrcrStack--;break;
 }
 Position++;
 if(BrcrStack!=0){
 Temp+=workingString.charAt(Position);
 }
 else {break;}
 }
 if(BrcrStack!=0){
 Main.Error=true;return("Error: missing \")\"");
 }
 separatedString.set(separatedString.size()-1, calculating(Temp));
 }
 else if (Character.isDigit(workingString.charAt(Position))){ //Bolting numbers
 while (((Position+1)<workingString.length())&&(Character.isDigit(workingString.charAt(Position+1))
 || workingString.charAt(Position+1)=='.') ){
 separatedString.set(separatedString.size()-1,
 separatedString.get(separatedString.size()-1)+workingString.charAt(Position+1));
 Position++;
 }
 try {
 Float.valueOf(separatedString.get(separatedString.size()-1));}
 catch(Exception e){
 Main.Error=true;return("Error: multiple points");
 }
 if(Negative){ //Negate parameter
 separatedString.set(separatedString.size()-1,Float.toString(-(Float.valueOf(separatedString.get(separatedString.size()-1)))));
 Negative=false;
 }
 }
 else if (Character.isAlphabetic(workingString.charAt(Position))){ //Bolting functions
 while (((Position+1)<workingString.length())
 &&(Character.isAlphabetic(workingString.charAt(Position+1))) ){
 separatedString.set(separatedString.size()-1,
 separatedString.get(separatedString.size()-1)+workingString.charAt(Position+1));
 Position++;
 }
 switch (separatedString.get(separatedString.size()-1)){ //Checking functions
 case "abs":break;
 case "sqrt":break;
 case "min":break;
 case "max":break;
 case "round":break;
 case "pow":break;
 case "exit":System.exit(0);
 default: {
 Main.Error=true;return("Error: unknown function \""+separatedString.get(separatedString.size()-1)+"\"");
 }
 }
 if ((Position+1)==workingString.length()||workingString.charAt(Position+1)!='(') //Parsing function parameters
 {
 Main.Error=true;return("Error: missing \"(\" after "+"\""+separatedString.get(separatedString.size()-1)+"\" "+ "function");
 }
 ArrayList<String> Temp = new ArrayList<>();
 int BrcrStack=1;
 Temp.add("");
 while (((Position+2)<workingString.length())){
 switch (workingString.charAt(Position+2)){
 case '(':BrcrStack++;break;
 case ')':BrcrStack--;break;
 case ',': if (BrcrStack==1){Temp.add("");Position++;continue;}break;
 }
 Position++;
 if(BrcrStack!=0){Temp.set(Temp.size()-1,Temp.get(Temp.size()-1)+Character.toString(workingString.charAt(Position+1)));}
 else {break;}
 }
 if(BrcrStack!=0){
 Main.Error=true;return("Error: missing \")\"");}
 Position++;
 float FuncArr[]=new float[Temp.size()];
 for (int i=0;i<Temp.size();i++){
 Temp.set(i, Calculating.calculating(Temp.get(i)));
 if (Temp.get(i).length()==0){
 Main.Error=true;return("Error: missing expression");}
 FuncArr[i]=Float.valueOf(Temp.get(i));
 }
 switch (separatedString.get(separatedString.size()-1)){ //Calculating functions
 case "abs":if (FuncArr.length==1) {
 separatedString.set(separatedString.size()-1,Float.toString(Math.abs(FuncArr[0])));}
 else {
 Main.Error=true;return("Error: \"abs\" function needs only one argument");}break;
 case "sqrt":if (FuncArr.length==1) {
 double a=FuncArr[0];
 separatedString.set(separatedString.size()-1,Double.toString(Math.sqrt(a)));}
 else {
 Main.Error=true;return("Error: \"sqrt\" function needs only one argument");}break;
 case "min":separatedString.set(separatedString.size()-1,Float.toString(Min(FuncArr))) ;break;
 case "max":separatedString.set(separatedString.size()-1,Float.toString(Max(FuncArr))) ;break;
 case "round":if (FuncArr.length==1) {
 separatedString.set(separatedString.size()-1,Float.toString(Math.round((FuncArr[0]))));}
 else {
 Main.Error=true;return("Error: \"round\" function needs only one argument");}break;
 case "pow":if (FuncArr.length==2) {
 separatedString.set(separatedString.size()-1,Double.toString(Math.pow(FuncArr[0],FuncArr[1])));}
 else {
 Main.Error=true;return("Error: \"pow\" function needs only two arguments");}break;
 }
 }
 Position++;
 }
 ArrayList<String> ResultArr = new ArrayList<>();
 if(Negative){ //Negate parameter
 separatedString.set(separatedString.size()-1,Float.toString(-(Float.valueOf(separatedString.get(separatedString.size()-1)))));
 }
 ArrayList<String> Stack = new ArrayList<>();
 int i=0;
 while (i<separatedString.size()){ //Converting expression to postfix notation
 if (separatedString.get(i).length()==1&&(separatedString.get(i).charAt(0)=='+'||separatedString.get(i).charAt(0)=='-'||separatedString.get(i).charAt(0)=='/'||separatedString.get(i).charAt(0)=='*')){
 if(Stack.size()==0){
 Stack.add(separatedString.get(i));
 }else {
 if ((separatedString.get(i).charAt(0) == '+' || separatedString.get(i).charAt(0) == '-') && (Stack.get(Stack.size() - 1).charAt(0) == '*' || Stack.get(Stack.size() - 1).charAt(0) == '/')) {
 Stack.add(separatedString.get(i));
 } else {
 Stack.add(separatedString.get(i));
 Stack.add(separatedString.get(i+1));
 i++;
 while (Stack.size() > 0) {
 ResultArr.add(Stack.get(Stack.size()-1));
 Stack.remove(Stack.size() - 1);
 }
 }
 }
 }
 else{
 ResultArr.add(separatedString.get(i));
 }
 i++;
 }
 while (Stack.size() > 0) {
 ResultArr.add(Stack.get(Stack.size()-1));
 Stack.remove(Stack.size() - 1);
 }
 i=0;
 while (ResultArr.size()>1){ //Calculating postfix expression
 if (ResultArr.get(i).length()==1&&(ResultArr.get(i).charAt(0)=='+'||ResultArr.get(i).charAt(0)=='-'||ResultArr.get(i).charAt(0)=='/'||ResultArr.get(i).charAt(0)=='*')){
 switch (ResultArr.get(i).charAt(0)){
 case '+':ResultArr.set(i-2,Float.toString(Float.valueOf(ResultArr.get(i-2))+Float.valueOf(ResultArr.get(i-1))));ResultArr.remove(i);ResultArr.remove(i-1);break;
 case '-':ResultArr.set(i-2,Float.toString(Float.valueOf(ResultArr.get(i-2))-Float.valueOf(ResultArr.get(i-1))));ResultArr.remove(i);ResultArr.remove(i-1);break;
 case '*':ResultArr.set(i-2,Float.toString(Float.valueOf(ResultArr.get(i-2))*Float.valueOf(ResultArr.get(i-1))));ResultArr.remove(i);ResultArr.remove(i-1);break;
 case '/':ResultArr.set(i-2,Float.toString(Float.valueOf(ResultArr.get(i-2))/Float.valueOf(ResultArr.get(i-1))));ResultArr.remove(i);ResultArr.remove(i-1);break;
 }
 i=0;
 }
 i++;
 }
 String Result="";
 for (String aResultArr : ResultArr) {
 Result += aResultArr;
 }
 if (Float.valueOf(Result)==Math.floor(Float.valueOf(Result))){
 Result=Integer.toString(Math.round(Float.valueOf(Result)));
 }
 return Result;
 }
 static float Min(float Arr[]){
 float Min=Arr[0];
 for (int i=1;i<Arr.length;i++){
 if (Arr[i]<Min){Min=Arr[i];
 }
 }
 return Min;
 }
 static float Max(float Arr[]){
 float Max=Arr[0];
 for (int i=1;i<Arr.length;i++){
 if (Arr[i]>Max){Max=Arr[i];
 }
 }
 return Max;
 }
}

Answer 1

Formatting

• Your variable names should start with a lower case letter by convention.
• Try to have spaces around operators (+, == etc.)
• Try to have spaces after if, while etc.
• don't have superlong lines. Somebdoy recommends hardlimit on 80 chars, somebody a soft limit on 120 chars, it doesn't matter. Simply try to have shorter lines if it's possible.
• generally, these formatting conventions are good to follow (As a baseline, not as a hard rule. They are quite aged, don't contain information about some of the newer Java features (annotations, generics...), enforce the nowadays-controversial spaces instead of tabs and a hard limit of 80 chars-per line.)
• instead of ArrayList<Something> list = new ArrayList<>();, use List<Something> list = new ArrayList<>(); See here or here.
• anyway, you got the formatting pretty much right

Code

1. This:

   String result = "";
   for (String aResultArr : resultArr) {
    result += aResultArr;
   }
   

   is horribly wrong. Strings in Java are immutable, which means that you can't add anything to an existing String. What happens instead is that a new String is allocated and all the characters are copied. This is a killer when used in a loop.

   The right way is to use a StringBuilder:

   StringBuilder resultBuilder = new StringBuilder();
   for (String aResultArr : resultArr) {
    resultBuilder.append(aResultArr);
   }
   String result = resultBuilder.toString();
   

   By the way, "some" + "thing" gets internally compiled to new StringBuilder().append("some").append("thing").toString() anyway (because of the String immutability I talked about).

2. You should generally split your code to have fewer loops, less indenting, much more methods. That way, it will be a lot more readable, maintainable (and therefore less error-prone). For example, this code in the main parsing while loop:

   // reducing spaces
   if (workingString.charAt(position) == ' ') {
    while ((position < workingString.length())
    && (workingString.charAt(position) == ' ')) {
    position++;
    }
   }
   

   This could be a separate method skipSpaces().

   Or, even better, move it out from the main loop. Do you want to strip the user input of all spaces? Do it right away when you read it!

   // Input a string to work with it, strip it of all spaces
   String stringForProcessing = read.readLine().replace(" ", "");
   

   Or, again, in a method, so it's possible to have a high-level overview over the code:

   // Input a string to work with it
   String stringForProcessing = read.readLine();
   stringForProcessing = stripOfSpaces(stringForProcessing);
   

3. Comment your code. Don't explain what it does. For example,

   // Calculating
   String output = Calculating.calculating(stringForProcessing);
   

   this one is pretty redundant. The code is self-explanatory. Or at least should be (again, split it into multiple methods).

   So don't explain what. Explain why.

   //checking value for negativity
   if ((workingString.charAt(position) == '-')
    && (position == 0
    || workingString.charAt(position - 1) == '-'
    || workingString.charAt(position - 1) == '+'
    || workingString.charAt(position - 1) == '*'
    || workingString.charAt(position - 1) == '/')) {
   

   So this one checks for negativity. Cool. That comment should have been a method name private static boolean checkNegative(String workingString, int pos). Or something similar.

   The comment? Not needed.

   What I don't understand (usually until I dive really deep into the code) is why is the condition so long and why is it checking previous characters, too. That should be commented: // checking previous chars, because of ...

4. Cache often used values.

   If you wrote workingString.charAt(position - 1) four times in a condition, you probably should have used

   char previousChar = workingString.charAt(position - 1);
   

   And then use this cached value in your condition. Your lines will be shorter, your code will be more clear and faster.

   Same problem:

   if (Float.valueOf(result) == Math.floor(Float.valueOf(result))) {
    result = Integer.toString(Math.round(Float.valueOf(result)));
   }
   return result;
   

   Use:

   float numResult = Float.valueOf(result);
   if (numResult == Math.floor(numResult)) {
    result = Integer.toString(Math.round(numResult));
   }
   return result;
   

   This issue is all around your code and would help quite a lot when gotten right. However, don't obsess about caching too much - it can hurt readability when used excessively. And readability should be one of your main concerns.

5. If you want to use something as a stack, look for a implemetation of stack, don't use a List. Your code will be much more readable, because you won't have to call stack.get(stack.size() - 1), but will use stack.getLast() instead.

   Also, try to avoid java.util.Stack for reasons you don't yet understand (think "it's old"). Use a Deque interface and a ArrayDeque (preferred) or LinkedList implementation.

   Deque<String> stack = new ArrayDeque<>();
   

6. When done with this, instead of your old

   while (Stack.size() > 0) {
    ResultArr.add(Stack.get(Stack.size()-1));
    Stack.remove(Stack.size() - 1);
   }
   

   you'll end up with code like this:

   while (stack.size() > 0) {
    resultArr.add(stack.getLast());
    stack.removeLast();
   }
   

   While visibly better, it can still be improved to:

   while (!stack.isEmpty()) {
    resultArr.add(stack.removeLast());
   }
   

7. You're parsing strings to numbers, then performing operations on them and then converting them back to strings. Highly inefficient and confusing both for you and for me. Consider storing numbers as ... numbers. I admit that to do this effectively and nicely, you'll probably need to write some classes or enums. That is a step up from your current programming level, so don't bother with it until you're sure what you're doing at the current level. But it's something to consider. The same applies to operations which can be implemented as enums/classes.

Wow. I didn't expect this to get so long. It's definitely not all the things that could use some polishing, but it's what caught my eye while going through. I think it's enough for today :).

Answer 2

Slanec's answer explains it's all. But I would like to add some points.

1. If you are writing your code in any IDE, try to use some source code analyzer like PMD. It will complain about unnecessary code content.

2. Your boolean field Error doesn't change and always stays as false. So I think you don't need to use that field. So the code snippet

   if(Main.Error){ //return error report
    return(workingString);
   }
   

   will not execute ever and makes it unnecessary.

3. Try to DRY your code and remember to Do Only One Thing.