HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Palindrome Checker in Java

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
palindromecheckerjava

Problem

My objective is to take a string as input and determine if it is a palindrome. I'm wondering how I could improve this code (efficiency, bugs, etc). I'm relatively new to Java so I'm expecting that my code will contain some errors.

import java.util.Scanner;

    /**
     * Created by Adam on 6/5/2015.
     */
    public class Reversal {
        public static void main(String args[]) {
            String word = "";
            String reversed = "";
            Scanner in = new Scanner(System.in);
            System.out.println("Please enter the string: ");
            word = in.nextLine();
            int length = word.length();
            for(int i = length; i > 0; i--) {
                reversed += word.charAt(i-1);
            }
            if(removeSpace(word).compareTo(removeSpace(reversed)) == 0) {
                System.out.println("The string " + word + " is a palindrome");
            }
            else {
                System.out.println("The string " + word + " is not a palindrome");
            }
        }

        private static String removeSpace(String aword) {
         String buffer = "";
            for(int i = 0; i < aword.length(); i++)   {
             if(!(String.valueOf(aword.charAt(i)).compareTo(" ") == 0)) {
                 buffer += aword.charAt(i);
             }
         }
            return buffer;
        }
    }

Solution

Firstly, this is something I see a distressing amount in beginner Java programmers:

removeSpace(word).compareTo(removeSpace(reversed)) == 0


There's a method for checking if two Strings are equal. It's called equals. It'd be used like this:

removeSpace(word).equals(removeSpace(reversed))


If you wanna ignore case, you could use equalsIgnoreCase:

removeSpace(word).equalsIgnoreCase(removeSpace(reversed))


Secondly, a formatting tip: Except in method declarations or calls, always put a space before ( and a space after ). So, for example:

if(...) {


becomes

if (...) {


In your removeSpaces method:

  • In general, when find that you're doing it a lot, avoid using String += String. Instead, initialize a StringBuilder and append to it.



  • String#charAt() returns a char, which can be compared with ==; I'm not sure why you first turn it into a String, then use compareTo (which, again, should be equals, since you were comparing Strings for equality)



With those issues fixed, this is what the method looks like:

private static String removeSpace(String aword) {
     StringBuilder buffer = new StringBuilder();
        for (int i = 0; i < aword.length(); i++)   {
         if (aword.charAt(i) != ' ')) {
             buffer.append(aword.charAt(i));
         }
     }
     return buffer.toString();
}


(Note that I changed what would have been !(... == ...) to ... != ... and that ' ' uses single quotes.)

The whole function could be replaced with word.replaceAll(" ", ""), but that's no fun, so I'm gonna keep using yours through here. Besides, it's a good way to practice to write your own methods -- just remember, when you're writing production code, to do a thorough search of the docs first. If there's a version in the standard API, a whole gaggle of people have been optimizing it to high heaven, and it might even be implemented in native code. That alone would make it a bazillion times faster than anything you could write in pure Java.

You use removeSpaces in the comparison, which somewhat obfuscates what really happens: The String is checked for palindromity (word coined?) ignoring the spaces. It's clearer to move those method calls to when you get it (plus, this removes an extra function call, which is more efficient :D):

word = removeSpaces(in.nextLine());
// ...
if(word.equals(reversed)) { // Note that I'm not using compareTo here
// etc.


Thirdly, I'd recommend pulling the code that reverses your String into another method that takes a String argument and returns a String (and applying my earlier advice about StringBuilder):

public static String reverse(String word) {
    StringBuilder reversed = new StringBuilder(word.length);
    for(int i = word.length(); i > 0; i--) {
        reversed.append(word.charAt(i - 1));
    }
}


In that snippet above, I also removed length, since it's ultimately useless. You're only using it once, so there's no point in declaring a variable, and it's actually clearer to be able to directly see what String's length we're talking about -- you don't need to go back in the code to see what it means

With all this, your main becomes a bit clearer:

public static void main(String args[]) {
    String word = "";
    String reversed = "";
    Scanner in = new Scanner(System.in);
    System.out.println("Please enter the string: ");
    word = removeSpaces(in.nextLine());
    reversed = reverse(word);
    if (word.equals(reversed)) {
        System.out.println("The string " + word + " is a palindrome");
    }
    else {
        System.out.println("The string " + word + " is not a palindrome");
    }
}


We could clean it up a little more by moving your word and reversed declarations to where the variables are first assigned, rather than their own lines (which, incidentally, don't need the = "" because they aren't used before you assign them later):

public static void main(String args[]) {
    Scanner in = new Scanner(System.in);
    System.out.println("Please enter the string: ");
    String word = removeSpace(in.nextLine());
    String reversed = reverse(word);
    if (word.equals(reversed)) {
        System.out.println("The string " + word + " is a palindrome");
    }
    else {
        System.out.println("The string " + word + " is not a palindrome");
    }
}


This means that your whole class, all together, looks something like this:

```
public class Reversal {
public static String reverse(String word) {
StringBuilder reversed = new StringBuilder(word.length);
for (int i = word.length(); i > 0; i--) {
reversed.append(word.charAt(i - 1));
}
}

private static String removeSpace(String aword) {
StringBuilder buffer = new StringBuilder();
for (int i = 0; i < aword.length(); i++) {
if(aword.charAt(i) != ' ')) {
buffer.append(aword.charAt(i));
}

Code Snippets

removeSpace(word).compareTo(removeSpace(reversed)) == 0
removeSpace(word).equals(removeSpace(reversed))
removeSpace(word).equalsIgnoreCase(removeSpace(reversed))
private static String removeSpace(String aword) {
     StringBuilder buffer = new StringBuilder();
        for (int i = 0; i < aword.length(); i++)   {
         if (aword.charAt(i) != ' ')) {
             buffer.append(aword.charAt(i));
         }
     }
     return buffer.toString();
}
word = removeSpaces(in.nextLine());
// ...
if(word.equals(reversed)) { // Note that I'm not using compareTo here
// etc.

Context

StackExchange Code Review Q#92796, answer score: 19

Revisions (0)

No revisions yet.