Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/

Source Link

edited May 23, 2017 at 12:41

Community Bot

edited May 23, 2017 at 12:41

Community Bot

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 here or here.
anyway, you got the formatting pretty much right

This:
```
String result = "";
for (String aResultArr : resultArr) {
 result += aResultArr;
}
```
is horribly wrong. 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 implementation.
```
Deque<String> stack = new ArrayDeque<>();
```

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

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. The same applies to operations which can be implemented as enums/classes.

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

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 implementation.
```
Deque<String> stack = new ArrayDeque<>();
```

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

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.

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

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 implementation.
```
Deque<String> stack = new ArrayDeque<>();
```

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

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.

Source Link

answered Jul 27, 2013 at 13:56

Petr Janeček

answered Jul 27, 2013 at 13:56

Petr Janeček

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

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 implementation.
```
Deque<String> stack = new ArrayDeque<>();
```

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

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

lang-java