HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Credit card validator using Luhn's algorithm

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
creditvalidatorcardalgorithmusingluhn

Problem

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  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 = 644 && input3Chars = 51 && input2Chars = 16 && str.length() = 13 && str.length() <= 16) {
                return "Visa";
            }
        }
        return "Invalid Credit Card Type";
    }
}

Solution

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  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  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  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).

Code Snippets

/**
 * @param args the command line arguments
 * @throws java.io.FileNotFoundException
 */
for (int i = 0; i < array.length; i++) {
        array[i] = Integer.parseInt("" + reverse.charAt(i));
    }
if (str.length() >= 16 && str.length() <= 19) {
    return "MasterCard";
}
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;
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;

Context

StackExchange Code Review Q#59521, answer score: 13

Revisions (0)

No revisions yet.