0
\$\begingroup\$

I've seen variations of this assignment where you reverse a string to match it to the original to find out if it is a palindrome, but this version I made only iterates through half of the string comparing it in reverse and would terminate the loop as soon as it notices a difference between the mirrored char and the current one in the index loop. This causes fewer loops than a reversal of the string.

  • There is a statement with a ternary operator and a modulus that could be perceived as hard to read.

Is there a better way than comparing chars? If you saw this in a code test as an answer, what would you assess this as? Is the more complex line with the ternary operator showing a more advanced knowledge of options or is it only hard to read?

Any other comments are welcome!

public boolean checkPalindrome(String word) {
    boolean palindrome = true;
    int wordLength = word.length() - 1;
    // If the word is dividable by 2 return half else return half including the middle char
    int halfWord = word.length() % 2 == 0? (word.length()/2) : ((int) word.length()/2) + 1 ;
    for(int i = 0; i < halfWord; i++) {
        char first = word.toLowerCase().charAt((wordLength - i));
        char last = word.toLowerCase().charAt(i);
        if(!(first == last)) {
            palindrome = false;
            break;
        }
    }
    return palindrome;
}
\$\endgroup\$
1
  • \$\begingroup\$ IMHO the speed increase is so small and it is not really worth the reduction in code readability. This is also evident in that you have had to comment the code to describe what it is doing. \$\endgroup\$
    – mbx-mbx
    Commented Oct 15, 2019 at 10:41

3 Answers 3

3
\$\begingroup\$

Your basic approach looks good. However I noticed a few things:

You're not checking for strings with 0 characters.

The ternary statement adjusting for odd or even length is unnecessary. Simply dividing by 2 and adding 1 will work for both odd or even lengths.

Inside the loop, you're calling toLower() twice on each iteration. This is quite inefficient. Storing the word converted to lower case in a variable and using that instead of the passed string would be much more efficient.

When testing for inequality it is usually better to use the inequality operator(!=) instead of not(!) and equals(==). It's more concise and easier to read.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Simply dividing by 2 as the middle letter is its own 1 letter "palindrome" \$\endgroup\$
    – Joop Eggen
    Commented Oct 15, 2019 at 11:06
  • \$\begingroup\$ @JoopEggen - noted \$\endgroup\$
    – user33306
    Commented Oct 15, 2019 at 11:15
  • \$\begingroup\$ Thank you, these were some good pointers! \$\endgroup\$
    – Gemtastic
    Commented Oct 15, 2019 at 11:22
2
\$\begingroup\$
    boolean palindrome = true;

While there are some who swear by this formulation, I find it just makes things more complicated. You don't need a variable to track whether it is a palindrome. If you find a pair of letters that don't match, you can return false immediately. If your method is so long that this is confusing, then it should probably be broken into multiple methods anyway.

I find this easier and more readable with two variables. Putting this advice together with the suggestion to use lower case once rather than twice per iteration (see the @tinstaafl answer):

    String normalized = word.toLower();
    for (int i = 0, j = normalized.length() - 1; i < j; i++, j--) {
        if (normalized.charAt(i) != normalized.charAt(j)) {
            return false;
        }
    }

    return true;

This assumes that zero length strings are supposed to be palindromes. If not, you could check for that condition at the beginning of the method.

This code is both shorter and simpler, which makes it easier to read.

You can do other normalizations to the string. For example, it would be reasonable to remove whitespace and punctuation. E.g. this code would currently fail on the input "Madam, I'm Adam." Because the spaces and punctuation won't match (and the capitalization, but you fix that). Of course, it's possible that the task requirements will only give you strings without such extraneous characters. But then why is it giving you a mix of upper and lower case?

    String normalized = word.toLower().replaceAll("[^\\pL]+", "");

This will remove everything but letters from the string. So the previous example would become "madamimadam" which would match as a palindrome. Which makes sense, as it is one of the classic examples of a palindrome.

See also

\$\endgroup\$
1
\$\begingroup\$

A common problem for "western" programmers is the inability to process letters that consist of multiple characters. Your approach will fail if it encounters one of those.

Unfortunately, the String class does not contain a reverse operation so once you have stripped the original string from punctuation and whitespace and converted it to upper or lower case, you have to go through StringBuilder:

final StringBuilder reverse = new StringBuilder(original).reverse();
return reverse.toString().equals(original);

There is no point in reinventing the wheel and making it square. :)

If this was an assignment in array or string manipulation, you can give this as a feedback to your professor.

\$\endgroup\$
1
  • \$\begingroup\$ Reinventing the wheel in this case is for fun, but it's a good point if you want to do the reverse string version to just use a SB. I will keep that in mind, thank you! \$\endgroup\$
    – Gemtastic
    Commented Oct 16, 2019 at 8:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.