While your solution works, it's very poorly written.
Single responsibility principle
It's good that you separated the user input part and the rest.
But you could go further.
The PalCheck
method has too many responsibilities:
- It checks if the input is a palindrome
- It prints the result
It would be better to use separate methods for these.
An intuitive name for a method that checks if a string is a palindrome or not would be isPalindrome
.
A void checkPalindrome
method could call isPalindrome
and print the result.
Unit testing
One of the great advantages of separating responsibilities is testability.
You should write unit tests to verify your implementation easily,
which is only possible when methods have a single responsibility.
Some example unit tests:
@Test
public void test_hello_is_not_palindrome() {
assertFalse(isPalindrome("hello"));
}
@Test
public void test_hello_olleh_is_palindrome() {
assertTrue(isPalindrome("hello olleh"));
}
@Test
public void test_aba_is_palindrome() {
assertTrue(isPalindrome("aba"));
}
@Test
public void test_x_palindrome() {
assertTrue(isPalindrome("x"));
}
@Test
public void test_xx_palindrome() {
assertTrue(isPalindrome("xx"));
}
@Test
public void test_empty_palindrome() {
assertTrue(isPalindrome(""));
}
Bad practices and inefficiencies
Instead of this:
end = end + result.charAt(i);
This is shorter:
end += result.charAt(i);
However, note that string concatenation in a loop is known to be inefficient and a bad practice. You should use a StringBuilder
for this kind of thing.
It would be better to use braces for the body of the for
loop and the if-else
statements too, even if the body is a single statement.
Not using braces consistently everywhere can potentially lead to mistakes and horrible bugs.
Instead of using result.equalsIgnoreCase(end)
,
it would be more efficient to convert the input to lowercase early,
and in the end use simple .equals
in the comparison.
Naming
Methods should be named with camelCase
, not PascalCase
.
result
and end
are very poor names.
They don't describe the purpose of the variables.
Suggested implementation
Here's an alternative implementation that's much more efficient:
private boolean checkPalindrome(String string) {
String text = string.replaceAll(" ", "").toLowerCase();
int len = text.length();
for (int i = 0; i < len / 2; ++i) {
if (text.charAt(i) != text.charAt(len - 1 - i)) {
return false;
}
}
return true;
}