Spot The Bug: The Luhn Algorithm Validator

The bugs are creeping back, and thankfully, so is Spot The Bug. No rest for the wicked!

Come out, come out,
wherever you are...

Two years back, I wrote some buggy code for a Luhn algorithm validator, in Java. For those who don't know what the hell a Luhn algorithm is, it's a surface-level check to see if an entered credit card number is actually a valid credit card number.

Now that I look at it, I don't know what the heck I was thinking that time. I wouldn't have written it like that, say, last week.

The user is supposed to be prompted with an input box, to input his number. The input is then tested to ensure that it contains numbers only, and is 10 characters in length. If it passes this test, then I run it through the Luhn test. If not, the input box keeps prompting the user till the user inputs a value that is numeric and 10 characters in length.

Sound simple enough? No way I could screw that up, right? Nuh-uh. Think again.

What went wrong

Things worked fine when I entered a valid number. This is 10 characters, all numeric.

So is this.

It also worked when I entered a string that wasn't 10 characters, then entered a valid number.

But when I entered something alphanumeric, the input box would keep coming up even if I subsequently entered a valid number.

Why it went wrong

Here's the code. The problem wasn't in the isValidLuhn() function, so I've truncated the code for that (it's kind of long).

See, I had this bright idea to run the string through a For loop and grab the number of non-numeric characters, incrementing the variable alpha each time I found one. And at the end of it, if the variable alpha was more than 0 or the number of characters was not 10, the process repeated.

package luhn;
import javax.swing.*;

public class Main {
    public static void main(String[] args) {
                Boolean acceptable=false;
                String pin = "";
                int alpha = 0;
       
                while (!acceptable) {
                        pin=JOptionPane.showInputDialog(null, "Please enter a 12 digit number.", "Your Credit Card Number", JOptionPane.QUESTION_MESSAGE);
           
                        for (int i=0; i < pin.length(); i++) {
                            if (!Character.isDigit(pin.charAt(i))) {
                                    alpha++;
                            }
                        }
           
                        if (pin.length()==10&&alpha==0) acceptable=true;
                }
       
                if (isValidLuhn(pin)) {
                    System.out.println("Credit card number is valid");
                } else {
                    System.out.println("Credit card number is invalid");
                }   
    }
   
    public static boolean isValidLuhn(String str) {
        ...
    }
}

Now, this was just horribly inefficient and error-prone, but that aside, there was a glaring logical flaw in the design. If I entered a non-numeric character, the variable alpha would change to a non-zero value, and the variable acceptable would still be false, thus perpetuating the loop. But no matter what I entered later, alpha would either remain the same (if I entered a valid numeric string) or get incremented further (if I entered an alphanumeric string).

How I fixed it

Aside from rewriting this hideous thing and adopting a more modular format? Tempting, but a far quicker fix would be just to reset alpha to 0 at the beginning of the While loop!

package luhn;
import javax.swing.*;

public class Main {
    public static void main(String[] args) {
                Boolean acceptable=false;
                String pin = "";
                int alpha = 0;
       
                while (!acceptable) {
                        pin=JOptionPane.showInputDialog(null, "Please enter a 12 digit number.", "Your Credit Card Number", JOptionPane.QUESTION_MESSAGE);
                        alpha = 0;

                        for (int i=0; i < pin.length(); i++) {
                            if (!Character.isDigit(pin.charAt(i))) {
                                    alpha++;
                            }
                        }
           
                        if (pin.length()==10&&alpha==0) acceptable=true;
                }
       
                if (isValidLuhn(pin)) {
                    System.out.println("Credit card number is valid");
                } else {
                    System.out.println("Credit card number is invalid");
                }   
    }
   
    public static boolean isValidLuhn(String str) {
        ...
    }
}

Moral of the story

Boy, that really made me feel stupid. On the other hand, I'm glad to report that I code a lot better now than I did two years ago. Damn, that was horrible.

The takeaway for this is, if exiting a loop depends on a variable, always reset the variable before operating on it! Or, you know, write code in such a way that you don't have to.

If you get silly bugs sometimes, remember you're not aLuhn!
T___T