Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote a program for a college assignment, and I'd like to receive some pointers on making my code a little more efficient. It'd be much appreciated. I'm still new to coding, so I apologize if my code is horrendous.

package lab07;

import java.util.Scanner;
public class Lab07 {

public static int reverse(int number){

       int result = 0;
       while (number !=0){
           int remainder = number % 10;
           result = result * 10 + remainder;
           number = number / 10;
       }
       return result;
   }
   public static boolean isPalindrome(int input){

   int Palindrome = reverse(input);

   if (Palindrome == input){

       return true;          
   }
   else
       return false;
   }
    public static void main(String[] args) {

   int integer = 0;
   Scanner input = new Scanner(System.in);      
   System.out.print("Enter a positive, multi-digit integer: ");
   integer = input.nextInt();
   while (integer <= 9 && integer > 0)
   {
        System.out.println(integer + " is a single digit. Please re-enter another integer: ");
        integer = input.nextInt();
                if (isPalindrome(integer) && (integer > 0 && integer > 9))
           {
                System.out.println(integer + " is a palindrome");
                return;
           }
           else if (!isPalindrome(integer) && (integer > 0 && integer > 9))
           {
                System.out.println(integer + " is not a palindrome");
                return;
           }

   }
   while (integer < 0)
   {
       System.out.println(integer + " is negative. Please re-enter another integer: ");
       integer = input.nextInt();
                if (isPalindrome(integer) && (integer > 0 && integer > 9))
            {
                 System.out.println(integer + " is a palindrome");
                 return;
            }
            else if (!isPalindrome(integer) && (integer > 0 && integer > 9))
            {
                 System.out.println(integer + " is not a palindrome");
                 return;
            }
                 while (integer <= 9 && integer > 0)
                        {
                             System.out.println(integer + " is a single digit. Please re-enter another integer: ");
                             integer = input.nextInt();
                                     if (isPalindrome(integer) && (integer > 0 && integer > 9))
                                {
                                     System.out.println(integer + " is a palindrome");
                                     return;
                                }
                                else if (!isPalindrome(integer) && (integer > 0 && integer > 9))
                                {
                                     System.out.println(integer + " is not a palindrome");
                                     return;
                                }
                        }                   
   }
   while (integer > 9){
                if (isPalindrome(integer) && (integer > 0 && integer > 9))
                {
                     System.out.println(integer + " is a palindrome");
                     return;
                }
                else if (!isPalindrome(integer) && (integer > 0 && integer > 9))
                {
                     System.out.println(integer + " is not a palindrome");
                     return;
                }
   }

}

   }

Sorry I didn't specify this before but I'm in an Intro to Programming class, and our professor hasn't taught us how to reverse strings. These were his instructions:

The program should ask the end user to enter an integer. Use the isPalidrome method to invoke the reverse method, and report whether the integer is a palindrome. Any one-digit integer or negative integer should be rejected with the specific error message (negative or one digit), and then the program should ask the user to re-enter the integer. Hint: To reverse the digits of a number, try this routine:

int result = 0;                           
while (number != 0) {                   // e.g. number = 123
                                        // Iteration 1        Iteration 2    Iteration 3 
  int remainder = number % 10;          // remainder = 3      remainder = 2  remainder = 1 
  result = result * 10 + remainder;     // result = 3         result = 32    result = 321
  number = number / 10;                 // number = 12        number = 1     number = 0
}                                       // result contains the reverse of number  

He provided the reverse method for us, I wouldn't have know how to actually reverse the integer on my own to be honest. However I appreciate the help on determining the palindrome.

share|improve this question
add comment

5 Answers

up vote 3 down vote accepted

Apart from the other comments about how to calculate the input, I would also suggest that your main method is buggy, and not very 'pretty'. One the the tricks in programming that Java programmers seem to forget, is the do-while loop (note, C programmers use this all the time, and Java programmers do all sorts of crazy things to avoid it ...! ).

Consider the an altered main method

  • renamed Scanner to be scanner
  • renamed integer to be input.
  • use do-while for input validation
  • use 'simpler' printf for output
public static void main(String[] args) {

    int input = 0;
    Scanner scanner = new Scanner(System.in);      
    boolean ok = false;
    do {
        System.out.print("Enter a positive, multi-digit integer: ");
        input = scanner.nextInt();
        if (input < 0) {
            System.out.println(input + " is negative. Please re-enter another integer: ");
        } else if (input <= 9) {
            System.out.println(input + " is a single digit. Please re-enter another integer: ");
        } else {
            ok = true;
        }
    } while (!ok);

    System.out.printf("%d %s a palindrome\n", input, isPalindrome(input) ? "is" : "is not");
    scanner.close();

}

I should add that having 'return' in a main method is unconventional.... not 'wrong' but it probably means you are doing something weird...

share|improve this answer
    
How do you code isPalindrom(Integer) method? - Why are you transforming the input in a int? keep it as String : (s==null || s.startwith('-') || s.length <2 ) --> false; is also a good test to protect StringBuilder method –  cl-r Nov 5 '13 at 12:56
    
Other people have already suggested answers for those questions. My notes are purely on the main method. As for your specific questions, I have used the OP's isPalindome() method and I think it is a better method than using String (since it's easier to validate the int). Also, I have just copy/pasted the OP's code and moved it, keeping as much of his 'style' as I can. It is not my intention to criticize or compete against other answers here –  rolfl Nov 5 '13 at 13:05
1  
I wouldn't suggest using do-while here. Not that do-while is bad as such, but having an ok flag isn't any better. To me the approach by @raistlinthewizard (using continue) is way more elegant. –  Ihor Kaharlichenko Dec 17 '13 at 7:16
    
The initial prompt belongs before the loop. –  200_success Dec 17 '13 at 10:50
add comment

To detect palindrome :

final String s = input + "";
return new StringBuilder(s).reverse().toString().equals(s);
share|improve this answer
    
Sorry I didn't specify this before but I'm in an Intro to Programming class, and our professor hasn't taught us how to reverse strings, and he provided the reverse method for us. So using your answer wouldn't make sense, but regardless thank you for your help. –  DeathWish Nov 5 '13 at 15:37
add comment

First: There is a wrong template in your code:

 if (isPalindrome(integer) && (integer > 0 && integer > 9))
       {
            System.out.println(integer + " is a palindrome");
            return;
       }
       else if (!isPalindrome(integer) && (integer > 0 && integer > 9))
       {
            System.out.println(integer + " is not a palindrome");
            return;
       }
  1. (integer > 0 && integer > 9) is equals to integer > 9
  2. The isPalindrome(integer) function used 2 times, but only need to invoke it 1 times per iteration.

Modified code:

 final boolean inputIsPalindrome = isPalindrome(integer);
 if (inputIsPalindrome && integer > 9))
   {
        System.out.println(integer + " is a palindrome");
        return;
   }
   else if (!inputIsPalindrome && integer > 9)
   {
        System.out.println(integer + " is not a palindrome");
        return;
   }

Here 2 problemes still there:

  1. checking double times the integer's value.
  2. Here you need only "if {} else {}" construct, because if the first true, the second is false.

So the re-modified code:

 if (integer > 9) {
   if (isPalindrome(integer)) {
        System.out.println(integer + " is a palindrome");
        // don't need "else" if returning here
        return;
   }

   System.out.println(integer + " is not a palindrome");
   return;
 }

More:

  1. You have a lot of loops to get the user input, but you only need one.
  2. In Java variables use CamelCase beginning with lower case character.
  3. The variable names have to tell more about itself.
  4. Handling input with wrong type is necessary.
  5. In this case I prefer reverse string instead of compute the reverse of the integer.

I've wrote your program here:

package lab07;

import java.util.Scanner;

public class Lab07 {

public static void main(String[] args) {

    System.out.print("Enter a positive, multi-digit integer: ");
    final String inputIntegerAsString = readInputIntegerAsString();

    if (isPalindrome(inputIntegerAsString)) {
        System.out.println(inputIntegerAsString + " is a palindrome");
        return;
    }

    System.out.println(inputIntegerAsString + " is not a palindrome");
}

private static String readInputIntegerAsString() {
    try (final Scanner scanner = new Scanner(System.in)) {
        // Instead of "true" you can use an exit condition, but in
        // this simple case I don't think to care about that.
        while (true) {
            final String line = scanner.nextLine();

            int inputInteger = 0;
            try {
                inputInteger = Integer.parseInt(line);
            } catch (final NumberFormatException e) {
                System.out.println("Invalid input. Please re-enter another integer: ");
                continue;
            }

            if (inputInteger < 10) {
                if (inputInteger < 0) {
                    System.out.println(inputInteger + " is negative. Please re-enter another integer: ");
                    continue;
                }

                System.out.println(inputInteger + " is a single digit. Please re-enter another integer: ");
                continue;
            }
            return line;
        }
    }
}

private static boolean isPalindrome(final String originalInputAsString) {
    final String reversedString = reverseString(originalInputAsString);

    return reversedString.equals(originalInputAsString);
}

private static String reverseString(final String originalString) {
    final StringBuilder stringBuilder = new StringBuilder(originalString);
    final String reversedString = stringBuilder.reverse().toString();

    return reversedString;
}
}
share|improve this answer
    
Make inputInteger final and do not initialize it before try clause. –  Ihor Kaharlichenko Dec 17 '13 at 7:25
add comment

Adding a second answer here because this answer is directly targetting your actual question: "How to make the palindrome program more efficient". ince the fastest answer I tested has not been mentioned yet in this review, I figure it is worth outlining here...

Personally I feel that the int approach to dealing with it is more intuitive, but I set out to prove this, and I was suprised, so, here is my test program....

First, it generates a million integer values stored as Strings in a char[] array. It then repeatedly 'scans' those values and checks each value to see whether it is a palindrome. In the spirit of the initial program, the test here is really whether converting the value to an int and then doing numberical manipulation is better than keeping the value as a String, and doing String comparisons..... I chose two different mechanisms for String compares....

Bottom line is that keeping the values as Strings is about 50% faster... huh. Half the time.

The most efficient method is:

public static boolean isPalindromeChar(final String input){
    int front = 0;
    int rear = input.length() - 1;
    while (front < rear) {
        if (input.charAt(front++) != input.charAt(rear--)) {
            return false;
        }
    }
    return true;
}

which is (on my machine) about 15% faster than:

public static boolean isPalindromeSBuilder(final String input){
    return input.equals(new StringBuilder(input).reverse().toString());
}

Doing Integer-based processing is significantly slower, probably because there's a lot of overhead in the Scanner.nextInt() method.

Here is my full code, but my sunmmary output is:

Average integer took 1.0457 seconds,
average string took 0.7060 seconds,
average char took 0.6139 seconds

Obviously, to benchmark this process I had to take some liberties, and the code is not a great solution to the original problem, but, as for the most efficient way to calculate palindromes from a Scanner, it is clearly to keep the valeus as Strings, and to do char-based comparisons.....

import java.io.CharArrayReader;
import java.io.CharArrayWriter;
import java.io.IOException;
import java.util.Random;
import java.util.Scanner;

public class Lab07 {

    public static int reverse(int number){

        int result = 0;
        while (number !=0){
            int remainder = number % 10;
            result = result * 10 + remainder;
            number = number / 10;
        }
        return result;
    }
    public static boolean isPalindromeInt(final int input){
        return input == reverse(input);
    }

    public static boolean isPalindromeSBuilder(final String input){
        return input.equals(new StringBuilder(input).reverse().toString());
    }

    public static boolean isPalindromeChar(final String input){
        int front = 0; int rear = input.length() - 1;
        while (front < rear) {
            if (input.charAt(front++) != input.charAt(rear--)) {
                return false;
            }
        }
        return true;
    }

    private static final void palindromeInteger(char[] data, int count) {
        CharArrayReader reader = new CharArrayReader(data);
        int palcount = 0;
        Scanner scanner = new Scanner(reader);
        while (count > 0) {
            int input = 0;
            boolean ok = false;
            do {
                //System.out.print("Enter a positive, multi-digit integer: ");
                input = scanner.nextInt();
                count--;
                if (input < 0) {
                    System.out.println(input + " is negative. Please re-enter another integer: ");
                } else if (input <= 9) {
                    System.out.println(input + " is a single digit. Please re-enter another integer: ");
                } else {
                    ok = true;
                }
            } while (!ok);

            if (isPalindromeInt(input)) {
                palcount++;
            }
        }
        System.out.printf("%d palindromes\n", palcount);
        scanner.close();
    }

    private static final void palindromeString(char[] data, int count) {
        CharArrayReader reader = new CharArrayReader(data);
        int palcount = 0;
        Scanner scanner = new Scanner(reader);
        while (count > 0) {
            String input = null;
            boolean ok = false;
            do {
                //System.out.print("Enter a positive, multi-digit integer: ");
                input = scanner.next();
                count--;
                if (input.startsWith("-")) {
                    System.out.println(input + " is negative. Please re-enter another integer: ");
                } else if (input.length() < 2) {
                    System.out.println(input + " is a single digit. Please re-enter another integer: ");
                } else {
                    ok = true;
                }
            } while (!ok);

            if (isPalindromeSBuilder(input)) {
                palcount++;
            }
        }
        System.out.printf("%d palindromes\n", palcount);
        scanner.close();
    }

    private static final void palindromeChar(char[] data, int count) {
        CharArrayReader reader = new CharArrayReader(data);
        int palcount = 0;
        Scanner scanner = new Scanner(reader);
        while (count > 0) {
            String input = null;
            boolean ok = false;
            do {
                //System.out.print("Enter a positive, multi-digit integer: ");
                input = scanner.next();
                count--;
                if (input.startsWith("-")) {
                    System.out.println(input + " is negative. Please re-enter another integer: ");
                } else if (input.length() < 2) {
                    System.out.println(input + " is a single digit. Please re-enter another integer: ");
                } else {
                    ok = true;
                }
            } while (!ok);

            if (isPalindromeChar(input)) {
                palcount++;
            }
        }
        System.out.printf("%d palindromes\n", palcount);
        scanner.close();
    }

    public static void main(String[] args) throws IOException {

        System.out.println("Generating values");

        CharArrayWriter caw = new CharArrayWriter();
        Random rand = new Random();
        final int numbers = 1000000;
        for (int i = 0; i < numbers; i++) {
            caw.write(rand.nextInt(numbers) + "\n");
        }
        caw.close();
        final char[] data = caw.toCharArray();

        System.out.println("Generated " + numbers + " values");

        long inttimes = 0L;
        long strtimes = 0L;
        long chrtimes = 0L;
        for (int loop = 0; loop < 11; loop++) {
            long intstart = System.nanoTime();
            palindromeInteger(data, numbers);
            if (loop > 0) {
                inttimes += System.nanoTime() - intstart;
            }
            long strstart = System.nanoTime();
            palindromeString(data, numbers);
            if (loop > 0) {
                strtimes += System.nanoTime() - strstart;
            }
            long chrstart = System.nanoTime();
            palindromeChar(data, numbers);
            if (loop > 0) {
                chrtimes += System.nanoTime() - chrstart;
            }
        }

        System.out.printf("Average %s took %.4f seconds, average %s took %.4f seconds, average %s took %.4f seconds\n",
                "integer", (inttimes / 10) / 1000000000.0,
                "string", (strtimes / 10) / 1000000000.0,
                "char", (chrtimes/ 10) / 1000000000.0);


    }

}
share|improve this answer
add comment
if (isPalindrome(integer) && (integer > 0 && integer > 9))
                    {
                         System.out.println(integer + " is a palindrome");
                         return;
                }

This piece should be moved to a separed method, for example:

private void checkInt(int value){
        if(value > 0 && value > 9){
        if (isPalindrome(value))
        {
            System.out.println(value + " is a palindrome");
        }
        else if (!isPalindrome(value) && (value > 0 && value > 9))
        {
            System.out.println(value + " is not a palindrome");
        }
        return;
    }

if (Palindrome == input){
           return true;}   
else{
           return false;}

This could be reduced to "return Palindrome == input"


Do not ever give variables such name as "integer","string" or any other class name.


Do not ever capitalize variable name: "int Palindrome"

share|improve this answer
    
Your code is longer than necessary. The condition value > 0 && value > 9 can be simplified to value > 9, you should not call isPalindrome twice. –  Roland Illig Dec 17 '13 at 5:51
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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