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.
  1. An N-Drome is a string that is the same forwards and backwards when split into n-character units.
  2. For example, a string would be a 1-drome if and only if it were a standard palindrome.
  3. An example of a 2-drome would be abcdab since its 2-character units are ab, cd, and ab, and its first and last 2-character units are identical.

package ndrome;

import java.util.Scanner;

public class NDrome {
    public static void main(String[] args) {
        System.out.print("enter a string: ");
        Scanner in = new Scanner(System.in);
        String original = in.nextLine();
        System.out.print("enter the n value: ");
        int n = in.nextInt();
        if(n-1>=original.length()){
            System.out.println("Error:N value is greater than string length! exiting...");
            System.exit(0);
        }
        int y = 2;
        here: for (int i = 0, j = original.length() - 1;;) {
            int x = n - 1;
            String a = "", b = "";
            while (x >= 0) {
                if (i >= original.length()) {
                    break here;
                } else {
                    a = a + original.charAt(i);
                    i++;
                    b = b + original.charAt(j - x);
                    x--;
                }
            }
            j=j-n;
            if (a.equals(b)) {
                y = 1;
            } else {
                y = 0;
                break;
            }
        }
        if (y == 1) {
            System.out.print("this is a " + n + "-drome");
        } else if (y == 0) {
            System.out.print("this is not a " + n + "-drome");
        }
    }
}
share|improve this question

2 Answers 2

up vote 5 down vote accepted

The good news is that your code appears to be correct. The bad news is that it is very hard to follow.

As @Jamal says, it is good practice to define a function rather than stuffing everything into main(). There are several reasons for that, including several reasons specific to your code:

  • Separation of concerns. Each function should do one thing; the function's name should describe its purpose. It seems obvious to have a main() that reads the input and prints the result. The check should be done in an isNDrome() function.
  • Code reuse. If everything is in main(), the only way to call your code is by running your program on the command line and writing to its standard input. Forget about ever putting a graphical or web interface on it. Your only reasonable code reuse mechanism is cut-and-paste.
  • Unit testing. As it stands, it's barely possible.
  • Simpler flow of control. You used a named break statement (with a label poorly named here, I might add). You also used a cryptically named variable (y) to store the result. When you discover that the string is not an N-drome, you have to set the flag (y = 0) then execute a jump (break). All of that would be cleaner with if your code were in an isNDrome() function: just return false as soon as you discover a mismatch.

As I mentioned, your code is hard to read. A significant contributor to the problem is the proliferation of cryptically named variables: y, i, j, x, a, b. Only n is self-explanatory due to the nature of the problem. You need to reduce the number of variables and name them better.

Performance-wise, string concatenation is considered to be a bad idea in Java. Since strings are immutable, each concatenation creates a new string. The innocuous-looking a = a + original.charAt(i) actually results in the following bytecode (which you can see by running javap -c NDrome):

 103: new           #16                 // class java/lang/StringBuilder
 106: dup           
 107: invokespecial #17                 // Method java/lang/StringBuilder."<init>":()V
 110: aload         8
 112: invokevirtual #18                 // Method java/lang/StringBuilder.append:(Ljava/lang/String;)Ljava/lang/StringBuilder;
 115: aload_2       
 116: iload         5
 118: invokevirtual #19                 // Method java/lang/String.charAt:(I)C
 121: invokevirtual #20                 // Method java/lang/StringBuilder.append:(C)Ljava/lang/StringBuilder;
 124: invokevirtual #21                 // Method java/lang/StringBuilder.toString:()Ljava/lang/String;
 127: astore        8

In other words, it's equivalent to

a = (new StringBuilder(a)).append(original.charAt(i)).toString();

Your code probably isn't performance-critical, but it would still be a good habit to avoid concatenating strings carelessly.

Proposed solution

The loop can be simplified to just half a dozen lines.

public static boolean isNDrome(CharSequence cs, int n) {
    int len = cs.length();

    if (n > len) {
        // Controversial, in my opinion.  Arguably, the function should
        // just return false (which it would do naturally if you just
        // remove this check altogether.).
        throw new IllegalArgumentException("N exceeds input length");
    }

    if (len % n != 0) {
        return false;
    }
    int chunks = len / n;
    for (int a = 0, z = chunks - 1; a < z; a++, z--) {
        // Compare chunk a, containing n characters from the first half
        // of the string, with chunk z, its counterpart from the second
        // half.
        CharSequence aChars = cs.subSequence(a * n, (a + 1) * n),
                     zChars = cs.subSequence(z * n, (z + 1) * n);
        if (!aChars.equals(zChars)) {
            return false;
        }
    }
    return true;
}

The code to call it can also look cleaner as a result.

public static void main(String[] args) {
    try (Scanner in = new Scanner(System.in)) {
        System.out.print("enter a string: ");
        String original = in.nextLine();
        System.out.print("enter the n value: ");
        int n = in.nextInt();

        try {
            System.out.printf("%s %s a %d-Drome\n",
                              original,
                              isNDrome(original, n) ? "is" : "is not",
                              n);
        } catch (IllegalArgumentException nExceedsLength) {
            System.out.printf("Error: %s! Exiting.\n",
                              nExceedsLength.getMessage());
        }
    }
}
share|improve this answer
    
Thank you for your insights. I will remember these points for the next time I code. You mentioned "javap -c NDrome". I reckon that is possible in command line. I am new to eclipse IDE. Can you please guide me how to do this in eclipse? –  GauravPandey Feb 17 at 18:12
1  
I don't use Eclipse myself, but this may help. –  200_success Feb 17 at 19:14

I'm not a Java programmer, but I'll point out some familiar syntactical things I see:

  • You should separate the logic by putting the checking code into a separate function, leaving main() to call the function, handle the input/output, and terminate when needed.

  • What is the purpose of y? From the looks of it, it's used to hold an "error condition" for indicating whether or not the inputted string is an N-drome. If so, I think this should be a boolean type and given a relevant name. Either way, y should be renamed to reflect its purpose.

    What about strings a and b? Are they the leftmost one and the rightmost one for the original string? You should write out the purpose of these single-char variables to give them more meaning.

  • The lack of an increment value in the for-loop doesn't look too clear. If someone wanted to see what the loop should do after each iteration, they would have to fish through the loop body. If the loop won't be incremented in each control path, then it may not be the best loop type to use.

  • I'm not familiar with break here, but it looks like a goto in C++. I don't think something like that should be done with a loop (or probably anything practical). Again, you may need to rethink how you approach this loop type and execution.

share|improve this answer
    
Y is a flag for my program. True I could have used a boolean type for better understanding. "a" and "b" are not single character variables, they hold a string and are for temporary use, so that the original string is not affected. Also I could not include the increment and condition statement in the loop as they have to incremented depending on the condition. Break is used to get out of the loop and prevent the program from doing unnecessary computations. Is there any way I can simplify the program with a simpler logic? –  GauravPandey Feb 17 at 2:48
    
I've never seen an N-drome before (though I was still able to answer), so I'll have to think about that. Again, I'm not fluent in Java, so there's not too much I can say. Please do wait around for others to provide an answer. –  Jamal Feb 17 at 2:55
    
Sure. But you pointed out a few good points. I'll keep those in my mind when I code next. Thank you. –  GauravPandey Feb 17 at 2:56
    
You're welcome. And welcome to Code Review! –  Jamal Feb 17 at 2:56

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.