Skip to main content
Code Review

Return to Answer

Added scanner try with resources
Source Link
AJNeufeld
  • 35.2k
  • 5
  • 41
  • 103

The functions countVowels() and countConsonants() do very similar things, but are implemented in entirely different ways. Could they not be written using the same method? Could they be defined one in terms of the other, such as consonants = string_length - vowels?

The statement str.toLowerCase(); does nothing. Well, it does do something ... it computes a lowercase version of str ... but doesn’t assign the result to anything, so the result is lost. You probably wanted str = str.toLowerCase();.

The class StringBuffer is deprecated; StringBuilder should be used in its place.

The StringBuffer#append() function returns itself to facilitate chained operation, like sb.append(x).append(y).append(x). The value returned should not be used for other purposes, such as printing. System.out.println(thirdForAdding.append(second)) is a side-effect in a print statement - a dangerous practice; unlearn it.

Constructing a StringBuffer (or a StringBuilder) to append one value is overkill. Just use regular string addition.

Explicit equality tests with true are unnecessary. You could simply write:

if (first.matches(firstRegex) && second.matches(secondRegex)) { ...

"Wrong" is only printed if the first test fails. If the second, third, or fourth tests fail, nothing is output.

The Scanner (and other Closable resources) should be closed to release resources immediately, instead of when garbage collected. The try-with-resources statement does this for you automatically & safely:

try (Scanner scn = new Scanner(System.in)) {
 ... use scanner here ...
}
... scanner has been closed after this point.

The functions countVowels() and countConsonants() do very similar things, but are implemented in entirely different ways. Could they not be written using the same method? Could they be defined one in terms of the other, such as consonants = string_length - vowels?

The statement str.toLowerCase(); does nothing. Well, it does do something ... it computes a lowercase version of str ... but doesn’t assign the result to anything, so the result is lost. You probably wanted str = str.toLowerCase();.

The class StringBuffer is deprecated; StringBuilder should be used in its place.

The StringBuffer#append() function returns itself to facilitate chained operation, like sb.append(x).append(y).append(x). The value returned should not be used for other purposes, such as printing. System.out.println(thirdForAdding.append(second)) is a side-effect in a print statement - a dangerous practice; unlearn it.

Constructing a StringBuffer (or a StringBuilder) to append one value is overkill. Just use regular string addition.

Explicit equality tests with true are unnecessary. You could simply write:

if (first.matches(firstRegex) && second.matches(secondRegex)) { ...

"Wrong" is only printed if the first test fails. If the second, third, or fourth tests fail, nothing is output.

The functions countVowels() and countConsonants() do very similar things, but are implemented in entirely different ways. Could they not be written using the same method? Could they be defined one in terms of the other, such as consonants = string_length - vowels?

The statement str.toLowerCase(); does nothing. Well, it does do something ... it computes a lowercase version of str ... but doesn’t assign the result to anything, so the result is lost. You probably wanted str = str.toLowerCase();.

The class StringBuffer is deprecated; StringBuilder should be used in its place.

The StringBuffer#append() function returns itself to facilitate chained operation, like sb.append(x).append(y).append(x). The value returned should not be used for other purposes, such as printing. System.out.println(thirdForAdding.append(second)) is a side-effect in a print statement - a dangerous practice; unlearn it.

Constructing a StringBuffer (or a StringBuilder) to append one value is overkill. Just use regular string addition.

Explicit equality tests with true are unnecessary. You could simply write:

if (first.matches(firstRegex) && second.matches(secondRegex)) { ...

"Wrong" is only printed if the first test fails. If the second, third, or fourth tests fail, nothing is output.

The Scanner (and other Closable resources) should be closed to release resources immediately, instead of when garbage collected. The try-with-resources statement does this for you automatically & safely:

try (Scanner scn = new Scanner(System.in)) {
 ... use scanner here ...
}
... scanner has been closed after this point.
Source Link
AJNeufeld
  • 35.2k
  • 5
  • 41
  • 103

The functions countVowels() and countConsonants() do very similar things, but are implemented in entirely different ways. Could they not be written using the same method? Could they be defined one in terms of the other, such as consonants = string_length - vowels?

The statement str.toLowerCase(); does nothing. Well, it does do something ... it computes a lowercase version of str ... but doesn’t assign the result to anything, so the result is lost. You probably wanted str = str.toLowerCase();.

The class StringBuffer is deprecated; StringBuilder should be used in its place.

The StringBuffer#append() function returns itself to facilitate chained operation, like sb.append(x).append(y).append(x). The value returned should not be used for other purposes, such as printing. System.out.println(thirdForAdding.append(second)) is a side-effect in a print statement - a dangerous practice; unlearn it.

Constructing a StringBuffer (or a StringBuilder) to append one value is overkill. Just use regular string addition.

Explicit equality tests with true are unnecessary. You could simply write:

if (first.matches(firstRegex) && second.matches(secondRegex)) { ...

"Wrong" is only printed if the first test fails. If the second, third, or fourth tests fail, nothing is output.

lang-java

AltStyle によって変換されたページ (->オリジナル) /