Palindrome checker using a sentinel loop

Question

This is my first submission and have completed one quarter of Java programming.

I have an assignment to create a Palindrome Checker. Fairly straight forward, I had that portion of the code figured out in the first hour. However, in typical fashion (for me) I want my code to do a bit more. This is where I run into issues.

I want this code to do the following:

• Take user input.
• Correctly identify if the input is a palindrome regardless of case or punctuation.
• Run in a loop so that multiple tests can be performed.

So far it works, I just don't know if I went about it the right way. Is this inefficient? Are there obvious rookie mistakes?

  /**
 * Created by Travis on 1/10/2015.
 */

import java.io.IOException;
import java.util.*;
import javax.swing.*;
import java.text.*;
import java.lang.StringBuilder;

public class Palindrome
{
    public static void main(String[] args) throws IOException //main class
    {
        String str = "", answer = "", test1 = "yes", test2 = "no";  //strings

        int len = 10;  //initial value for len so it doesn't trip the success if

        Scanner KB = new Scanner(System.in);  //user input

        System.out.println("Greetings, Welcome to the Palindrome Checker.\n" +  //initial greeting
                "Would you like to check a Palindrome? (Yes or no)");

        answer = KB.nextLine(); //input is for the sentinel program.

        if (!(answer.equalsIgnoreCase(test1) || answer.equalsIgnoreCase(test2)))  //error message in case user inputs incorrect string.
        {
            System.out.println("Error! You can only choose 'yes' or 'no'.  Please try again:");
            answer = KB.nextLine();  //allows for new answer
        }

        System.out.println(answer + " y"); //debugging so i can see answer

        while (answer.equalsIgnoreCase(test1)) //compares to test1 which is: yes.  as long as answer equals yes, the program should loop
        {
            System.out.println("Please provide a word or phrase:");

            str = KB.nextLine(); //prompt for palindrome

            String str2 = str.toLowerCase().replaceAll("\\s+", "").replaceAll("\\W+", "");  //converts input to a string that is a single group of chars with no space or punctuation

            System.out.println(str2);  //debug

            StringBuilder str1 = new StringBuilder(str2);  //takes the string and makes a stringbuilder so i can delete chars to test

            len = str1.length(); //sets for length.  This lets me account for any length of phrase

            System.out.println(len); //debug

            System.out.println(str1); //debug

            for (int i = 0; len >= 2; i++) //for loop to progressively test front and back letters and proceed if they are the same
            {
                char ch1 = str1.charAt(0);  //takes the letter at [0] and converts to char
                char ch2 = str1.charAt(len - 1);  //takes the last letter and converts to char

                System.out.println("The first letter in your phrase is: " + ch1);  // lists the letter at [0]
                System.out.println("The last letter in your phrase is: " + ch2);  //lists the letter at the end

                if (!(ch1 == ch2))  //if the front and back letter do not match, fails and prompts for new phrase
                {
                    System.out.println("Sorry, this phrase is not a palindrome. \n" +
                            "Would you like to try again?");
                    answer = KB.nextLine();
                    break;
                }

                else if (ch1 == ch2) //if front and back do match, removes front and back letters and updates stringbuilder
                {
                    System.out.println("Removing letters on each end of the phrase and performing " +
                            "new check:\n");
                    str1.deleteCharAt(len - 1);  //deletes the last letter from str1
                    str1.deleteCharAt(0);  //deletes first letter from str1
                    len = str1.length();  //updates len with the new length of str1
                    i++;
                }
            }

            if (len <= 1) //if the for loop successfully reduces stringbuilder to 1 or less characters, prompts for success
            {
                System.out.println("Congratulations, your phrase: '" + str + "' is a palindrome! \n\n" +
                        "Would you like to try again?");
                answer = KB.nextLine();
                len = 10; //resets len
            }
        }

        if (answer.equalsIgnoreCase(test2)) //ends sentinel loop.
        {
            System.out.println(answer + " N");
            System.exit(0);
        }
   }
}

Answer 1

I'll just elaborate a bit more on @Legato's point that "pretty much all your code is in main". Your program flow is now roughly like the following:

Greet user
(A) Ask whether to check for a palindrome or not
(B) Get input if "yes"
(C) Check for palindrome
(D) Print result
(A) Ask whether to check for a palindrome or not
(B) Get input if "yes"
(C) Check for palindrome
(D) Print result
...

As you can see, (A) and (B) can be done in its own method that returns a String, e.g.

private static String getInput() {
    // (A) prompt whether to check palindrome or not, while ensuring yes/no input
    if (answer.equals("no")) {
        return "";
    } else {
        // (B) answer = another prompt for a non-empty String
        return answer;
    }
}

(C) can simply be in its own method, that returns a simple boolean to indicate if the input is palindromic or not. All checks can be done within the method.

private static boolean isPalindromic(String input) {
    // (C) return true or false
}

As for (D), the console output should also be isolated from the palindromic checking, because both are mutually exclusive. Another (theoretical) way of looking at this is that you may want to replace the output with a file write-out, for example. Putting them all together, your main will probably look something like this:

public static void main(String[] args) {
    String input = "";
    while (!(input = getInput()).isEmpty()) {
        if (isPalindromic(input)) {
            System.out.println("Yes, this is palindromic.");
        } else {
            System.out.println("Sorry, this is not palindromic.");
        }
    }
}

Answer 2

Employ Java API

Why not keep it simple? StringBuilder has a built-in reverse method that you can use to make a method like:

private static boolean isPalindrome(String str) {
   return str.equalsIgnoreCase(
        new StringBuilder(str).reverse().toString()
   );
}

better yet extract the logic and have one method to reverse a string:

private static String reverse(String input) {
        return new StringBuilder(input).reverse().toString();
}

which would simplify the method to:

private static boolean isPalindrome(String str) {
       return str.equalsIgnoreCase(reverse(str));
}

Apply simple Regex functions for syntax requirements

Since you also want to ignore all punctuation instead of consecutively calling replaceAll for spaces and non-word characters you can just call (replaceAll("[^a-zA-Z]", "") which would, on top of removing all spaces and non-word characters as you desire, also discard numbers (anything that isn't a-z, A-Z). Though I don't know how I feel about "ra@!#33!cec!(*@--..!ar" being a valid palindrome.

Caveat: If you choose to go with the method the above, I'd also check if the string only contains otherwise ignored characters so you don't get false positives (empty or all nonsense strings reported as true).

Putting it all together the overall method would look like:

public static boolean isPalindrome(String str) {
        String possiblePalindrome = str.replaceAll("[^a-zA-Z]", "");
        return possiblePalindrome.length() == 0 ?
            false : possiblePalindrome.equalsIgnoreCase(reverse(possiblePalindrome));
}

On organization & programming approach

Building something like this for the sake of understanding is definitely something to be applauded. Good on you for doing so, and definitely keep it up! Do, however, note the importance of evaluating what's already available. The entire program is fully encompassed by the application of three functions the native libraries afford you, but this is clearly the small part of a greater whole.

In the future, consider doing a bit of research on what conventional libraries make available to you so you may focus on the implementation for your specific purpose rather than worrying about the minute details. It may not be obvious but this concentrated approach results in a better accrual of wisdom.

Separation of concerns

Pretty much all your code is in main(), it would benefit maintainability and readability if you moved some of the responsibilities outside. For instance, your entire for and while validation loops could just be one method that threw an IllegalArgumentException that could simply call the method again to ensure correct input.

Why do you import javax.swing.* and java.text.* libraries? They are unused.

On convention

It's Java standard to have the braces follow the class and method names, like so:

public class Palindrome {

This is the case anytime you use the curly braces, including your loops, with the additional caveat that else, catch, and other "paired" statements are preceded by their preceding statement's closing brace, like so:

if (condition) {
    // do something
} else {
    // do something else
}

Although it's a definite plus that you're consistent about it, this is a minor adjustment that would make your code more readable, especially if you intend to share it.

Answer 3

The code littered with System.out.println is messy and hard to read. Always try to decompose your code to smaller units (methods, classes) that each have a single responsibility.

Using StringBuilder.reverse as @Legato did is clever, but it probably defeats the purpose of the exercise. Here's another implementation that's still simple enough and doesn't cheat.

private boolean isPalindrome(String string) {
    String text = string.replaceAll("\\W+", "").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;
}

Particularly bad elements in your implementation:

• Excessive use of print statements (actually there shouldn't be any)
• Multiple .toLowerCase and .equalsIgnoreCase calls, when you could do .toLowerCase once at the very beginning, and then simply use .equals everywhere
• Since whitespace characters are also non-word characters, .replaceAll("\\s+", "").replaceAll("\\W+", "") can be simplified to .replaceAll("\\W+", "")

Answer 4

Imports:

• Organize your imports.
• Remove unused ones.
• Be specific with what import is required instead of using .*.

Nomenclature:

• KB? After much thought, it looks like it represents keyboard. Starting a variable name with Capital?
• answer = KB.nextLine()? Why not input and then find if its an answer or not.
• test1, test2? Instead have a function/ switch case to check for the valid inputs and if this needs to be used, define it as a static final constant or an enum.

Optimisation:

• Multiple equalsIgnoreCase. Instead an input variable converted to lowercase as per test1/2 can be used.
• If I enter anything other than yes or no, I cannot retry? Did you think it through? Is this expected. Do you want a yes to continue and rest all should exit? Do you want Yes/No to define the flow and rest will exit the program.
• If the first and last matches, just increment i instead of deleting the charaters and modifying the string. Check for charAt(i) and charAt(len-i-1).

Others (Suggestions) :

• Instead of multiple debugs, you could try Logger. (This incurs the cost of an additional library which is not required but its worth knowing its usage) or instead have a function say printLogs(String mesg, int logLevel) so that in case you wish to do not want the logs, you dont have to make multiple changes.
• len = 10 looks bad.
• main throws exception but its best if you catch it inside main so that the program doesn't exit suddenly incase of any sudden mishap. A log would do you more benefit in the catch.
• You as user to enter Yes or No and if he does make a mistake, you ask him to enter yes or no? A capital Y/N may confuse the user.
• Modularize the implementation into functions instead of putting everything in the main().

Answer 5

First, you should use descriptive variable names. 'test1' is not very descriptive of what it is used for.

Second, instead of initializing 'len' to 10, you should initialize it to -1. This would be better because it is an impossible value for a string's length.