Evaluating expressions with various mathematical methods

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 separatedString = new ArrayList<>();
int Position=0;
boolean Negative=false;
if(Main.Error){ //return error report
return(workingString);
}
while(Position Temp = new ArrayList<>();
int BrcrStack=1;
Temp.add("");
while (((Position+2) ResultArr = new ArrayList<>();
if(Negative){

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 list = new ArrayList<>();, use List list = new ArrayList<>(); See here or here.

• anyway, you got the formatting pretty much right

Code

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

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

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

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

-
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 im

StackExchange Code Review Q#29055, answer score: 4