Credit card validator using Luhn's algorithm

Question

I'm writing an algorithm to read from a file a list of numbers, and for each, determine if it is valid. If it is, then display which card type it is.

public class CreditCardValidator {

    /**
     * @param args the command line arguments
     * @throws java.io.FileNotFoundException
     */
    public static void main(String[] args) throws FileNotFoundException {
        Scanner scan = new Scanner(new File(args[0])); // read from a file
        while (scan.hasNextLine()) {
            String str = scan.nextLine();
            if (validate(str)) {
                System.out.println(creditCardType(str));
            } else {
                System.out.println("Invalid");
            }
        }
    }

    private static boolean validate(String str) {
        String reverse = new StringBuilder().append(str).reverse().toString();
        int[] array = new int[str.length()];
        for (int i = 0; i < array.length; i++) {
            array[i] = Integer.parseInt("" + reverse.charAt(i));
        }
        int sum = 0;
        for (int i = 0; i < array.length; i++) {
            if (i % 2 == 1) {
                array[i] *= 2;
                if (array[i] > 9) {
                    array[i] -= 9;
                }
            }
            sum += array[i];
        }
        return sum % 10 == 0;
    }

    private static String creditCardType(String str) {
        int input1Char = Integer.parseInt(str.substring(0, 1));
        int input2Chars = Integer.parseInt(str.substring(0, 2));
        int input3Chars = Integer.parseInt(str.substring(0, 3));
        int input4Chars = Integer.parseInt(str.substring(0, 4));
        int input6Chars = Integer.parseInt(str.substring(0, 6));

        if (input2Chars == 34 || input2Chars == 37) {
            if (str.length() == 15) {
                return "American Express";
            }
        }

        if (input4Chars == 6011
                || (input6Chars >= 622126 && input6Chars <= 622925)
                || (input3Chars >= 644 && input3Chars <= 649)
                || input2Chars == 65) {
            if (str.length() == 16) {
                return "Discover";
            }
        }

        if (input2Chars >= 51 && input2Chars <= 55) {
            if (str.length() >= 16 && str.length() <= 19) {
                return "MasterCard";
            }
        }

        if (input1Char == 4) {
            if (str.length() >= 13 && str.length() <= 16) {
                return "Visa";
            }
        }
        return "Invalid Credit Card Type";
    }
}

Answer 1

Comments

/**
 * @param args the command line arguments
 * @throws java.io.FileNotFoundException
 */

You're stating the obvious and forgetting the important things. Yes, args is the command line arguments, but that's standard for the Java main method. I'd suggest documenting which command line arguments you actually accept - namely only one, and it should be a file path!

Additionally, some of the code in the methods below looks like it could use some commenting as well. It looks complicated. Maybe I'll understand it if I look at it in greater detail...

Throw exceptions when it matters

    for (int i = 0; i < array.length; i++) {
        array[i] = Integer.parseInt("" + reverse.charAt(i));
    }

From your validate function. This will throw NumberFormatException if it fails to convert the character to an Integer. But if you throw NumberFormatException, the whole program grinds to a halt! If you encounter an unparseable character in a creditcard number, then obviously it's not a valid credit card number. Wrap it with try-catch and return false.

Readability

if (str.length() >= 16 && str.length() <= 19) {
    return "MasterCard";
}

When dealing with ranges, I prefer to write an if statement as a range, rather than two separate conditions. Consider if (16 <= str.length() && str.length() <= 19) instead. This goes for all cases where you have ranges like this.

Magic Numbers

Your third method is filled with magic numbers! What do 622126, 622925, 6011, 644, 649 and so on even mean?! In this case, I think they're all connected in some sort of way. You should add a comment in the code stating where you got the numbers from.

Simplifying the algorithm

    int[] array = new int[str.length()];
    for (int i = 0; i < array.length; i++) {
        array[i] = Integer.parseInt("" + reverse.charAt(i));
    }
    int sum = 0;
    for (int i = 0; i < array.length; i++) {
        if (i % 2 == 1) {
            array[i] *= 2;
            if (array[i] > 9) {
                array[i] -= 9;
            }
        }
        sum += array[i];
    }
    return sum % 10 == 0;

You're using two for loops, but don't use the result in the meantime. You end up only needing the sum of the contents, with some minor permutations. Let's simplify!

First, merge the two for loops.

    int[] array = new int[str.length()];
    int sum = 0;
    for (int i = 0; i < array.length; i++) {
        array[i] = Integer.parseInt("" + reverse.charAt(i));
        if (i % 2 == 1) {
            array[i] *= 2;
            if (array[i] > 9) {
                array[i] -= 9;
            }
        }
        sum += array[i];
    }
    return sum % 10 == 0;

Now that we've merged the two for loops we see that we store data in the array... but never use it afterwards! So all we really need is a single int value.

    int sum = 0;
    for (int i = 0; i < str.length(); i++) {
        int digit = Integer.parseInt("" + reverse.charAt(i));
        if (i % 2 == 1) {
            digit *= 2;
            if (digit > 9) {
                digit -= 9;
            }
        }
        sum += digit;
    }
    return sum % 10 == 0;

And there we go - it's a bit simpler now. You'll still have to add the try-catch that I suggested above yourself, though.

Other comments

String reverse = new StringBuilder().append(str).reverse().toString();

java.lang.StringBuilder can take a String as argument. Doing so would allow you to remove your append call, shortening the code, making it more readable (and potentially speeding things up slightly as you need 1 function call less, but that's negligible).

Answer 2

The two core functions can both be improved....

validate()

A validation function normally returns true, or false, but your function can also throw exceptions when there are illegal (non digit) characters, because of the Integer.parseInt(...).

Additionally, you can run Luhn's algorithm without the array.... and certainly without the reverse..... In fact, Luhn's is designed to run in-line, and not need to have any intermediate storage, which is why it is useful.

Your method:

private static boolean validate(final String str) {
    String reverse = new StringBuilder().append(str).reverse().toString();
    int[] array = new int[str.length()];
    for (int i = 0; i < array.length; i++) {
        array[i] = Integer.parseInt("" + reverse.charAt(i));
    }
    int sum = 0;
    for (int i = 0; i < array.length; i++) {
        if (i % 2 == 1) {
            array[i] *= 2;
            if (array[i] > 9) {
                array[i] -= 9;
            }
        }
        sum += array[i];
    }
    return sum % 10 == 0;
}

Can be replaced with the following method. You will notice that the if-logic has been entirely replaced with mathematics... allowing you to have just the one loop, and only the one valid-digit if statement. The three tricks are:

• the multiplication by zero to avoid the double
• the / 10 and % 10 to do the if double > 9 then double -= 9
• the use of the Character.digit() function to perform the digit-to-integer parsing (without throwing the exception).

The resulting code:

private static boolean validate(final String str) {
    final int offset = str.length() - 1;
    int sum = 0;
    for (int i = 0; i <= offset; i++) {
        final int d = Character.digit(str.charAt(offset - i), 10);

        if (d < 0) {
            // not a digit, value not valid
            return false;
        }
        final int v = d + (i % 2) * d;
        sum += v / 10 + v % 10;
    }
    return sum % 10 == 0;
}

creditCardType()

The creditCardType() function also has problems....

This function assumes the string will be a certain length, and fails if it is not.... the input "" will throw an IndexOutOfBounds exception....

I recommend working off some basics first:

public static String creditCardType(String number) {

    final int sz = number.length();
    if (!validate(number) || sz < 13) {
        return "Not a valid CC number";
    }

    switch (Character.digit(number.charAt(0))) {
        case 4:
             // Visa
             if (sz >= 13 && sz <= 16) {
                 return "Visa";
             }
             break;
        case 5:
             // Mastercard possibly....
             .....
        .....
     }

     return "Invalid Credit Card Type";
}

Answer 3

Wasteful repeated method calls

You're calling str.length() multiple times within the same scope. It would be better to call only once, save the result and reuse:

int length = str.length();

Input validation

As @Pimgd pointed out, you may get a NumberFormatException here:

array[i] = Integer.parseInt("" + reverse.charAt(i));

I propose validating like this:

int digit = reverse.charAt(i) - '0';
if (digit < 0 || digit > 9) {
    return false;
}

Naming

These methods could be named better:

• Instead of validate, how about isValid
• Instead of creditCardType, how about getCreditCardType

These variables could be named better:

• Instead of input1Char, how about first1
• Instead of input2Chars, how about first2
• Instead of input6Chars, how about first6
• ... and so on

Unit testing

It's important to test validators thoroughly. Consider adding unit tests, for example:

@Test
public void testValidVisa() {
    assertTrue(CreditCardValidator.isValid("1234567812342222"));
}

@Test
public void testVisa() {
    assertEquals("Visa", CreditCardValidator.getCreditCardType("4234567812342222"));
    assertEquals("Invalid Credit Card Type", CreditCardValidator.getCreditCardType("1234567812342222"));
}

@Test
public void testMaster() {
    assertEquals("MasterCard", CreditCardValidator.getCreditCardType("5134567812342222"));
    assertEquals("Invalid Credit Card Type", CreditCardValidator.getCreditCardType("51345678123422221111"));
}

@Test
public void testDiscover() {
    assertEquals("Discover", CreditCardValidator.getCreditCardType("6011513456781234"));
    assertEquals("Discover", CreditCardValidator.getCreditCardType("6221263456781234"));
    assertEquals("Discover", CreditCardValidator.getCreditCardType("6441263456781234"));
    assertEquals("Invalid Credit Card Type", CreditCardValidator.getCreditCardType("6741263456781234"));
}

These are just examples. Add more, try to cover all corner cases. Tests like these make refactoring easy too: once you have passing tests for all corner cases, you can boldly go and refactor, knowing that if something breaks, you'll know it immediately.