Loop in Credit Card Validation in java

Question

I am a high school student in an introductory Computer Science course. Our assignment was the following:

The last digit of a credit card number is the check digit, which protects against transcription errors such as an error in a single digit or switching two digits. the following method is used to verify actual credit card numbers but, for simplicity, we will describe it for numbers with 8 digits instead of 16:

• Starting from the rightmost digit, form the sum of every other digit. For example, if the credit card number is 4358 9795, then you form the sum 5+7+8+3 = 23.
• Double each of the digits that were not included in the preceding step. Add all the digits of the resulting numbers. For example, with the numbers given above, doubling the digits, starting with the next-to-last one, yields 18 18 10 8. Adding all the digits in these values yields 1+8+1+8+1+0+8=27.
• Add the sums of the two preceding steps. If the last digit of the result is 0, the number is valid. In our case, 23 + 27 = 50, so the number is valid.

Write a program that implements this algorithm. The user should supply an 8-digit number, and you should print out whether the number is valid or not. If it is not valid, you should print out the value of the check digit that would make the number valid.

I have everything done except for the part in bold. My code is listed below:

public class CreditCard 
{ 

    private String creditCardNumber;
    private boolean valid;
    private int checkDigit;
    int totalSum;

    /**
     * Constructor for objects of class CreditCard
     */
    public CreditCard(String pCreditCardNumber)
    {
        creditCardNumber = pCreditCardNumber;
        checkDigit = Integer.parseInt(pCreditCardNumber.substring(creditCardNumber.length() - 1));
        int sumOfDigits = checkDigit + Integer.parseInt(pCreditCardNumber.substring(6,7)) + Integer.parseInt(pCreditCardNumber.substring(3,4)) + Integer.parseInt(pCreditCardNumber.substring(1,2));
        int dig7 = Integer.parseInt(pCreditCardNumber.substring(7,8));
        int dig5 = Integer.parseInt(pCreditCardNumber.substring(5,6));
        int dig3 = Integer.parseInt(pCreditCardNumber.substring(2,3));
        int dig1 = Integer.parseInt(pCreditCardNumber.substring(0,1));

        String string7 = Integer.toString(dig7);
        int doubledDig7a = Integer.parseInt(string7.substring(0));
        int doubledDig7b = 0;
        if (dig7 * 2 >= 10)

        {
            doubledDig7a = Integer.parseInt(string7.substring(0));
            doubledDig7b = 0;
        }

        String string5 = Integer.toString(dig5);
        int doubledDig5a = Integer.parseInt(string7.substring(0));
        int doubledDig5b = 0;
        if (dig5 * 2 >= 10)

        {
            doubledDig5a = Integer.parseInt(string5.substring(0));
            doubledDig5b = 0;
        }

        String string3 = Integer.toString(dig3);
        int doubledDig3a = Integer.parseInt(string3.substring(0));
        int doubledDig3b = 0;
        if (dig3 * 2 >= 10)

        {
            doubledDig3a = Integer.parseInt(string3.substring(0));
            doubledDig3b = 0;
        }

        String string1 = Integer.toString(dig1);
        int doubledDig1a = Integer.parseInt(string1.substring(0));
        int doubledDig1b = 0;
        if (dig1 * 2 >= 10)

        {
            doubledDig1a = Integer.parseInt(string1.substring(0));
            doubledDig1b = 0;
        }


        int doubleDigits = doubledDig1a + doubledDig1b + doubledDig3a + doubledDig3b + doubledDig5a + doubledDig5b + doubledDig7a + doubledDig7b;

        totalSum = sumOfDigits + doubleDigits;

        if (totalSum % 10 == 0)
        {
            valid = true;
        }
        else
        {
            valid = false;
        }

    }

    public void makeItValid()
    {
       while (totalSum % 10 != 0)
       {
           checkDigit--;
           if (totalSum % 10 == 0)
           {
               break;
            }
        }
    }


    public boolean isItValid()
    {
        return valid;
    }
}

The loop is what I am having issues with. I always end up in an infinite loop whenever it compiles. It looks like everything should work, though. It's supposed to decrease the value of the check Digit (not increase so I don't end up with a check digit of 10 or higher), and then add that number back into the total sum until the total sum is divisible by 10, and then the loop would end. Is the type of loop I'm using wrong? Any advice would be appreciated.

Answer 1

Your problem is that both of your loop conditions involve totalSum but you only change checkDigit.

while (totalSum % 10 != 0)
{
    checkDigit--;
    if (totalSum % 10 == 0)
    {
        break;
    }
}

You either need to recalculate totalSum or change the condition to be based on checkDigit. If you want to loop and decrement like you are doing you will need to add a method that performs the algorithm and call it every time. The way you have your class outlined makes this very inconvenient because you don't convert the numbers.

public static int[] cardToNumbers(String cardText) {

    // \D is regex for non-digits
    cardText = cardText.replaceAll("\\D", "");

    int[] cardNumbers = new int[cardText.length()];

    // convert unicode to corresponding integers
    for (int i = 0; i < cardText.length(); i++)
        cardNumbers[i] = cardText.charAt(i) - '0';

    return cardNumbers;
}

public static int calcTotalSum(int[] cardNumbers) {

    int sum = 0;

    /* "every other one" loops
     *
     * I recommend against the "mod 2 index" scheme
     * i % 2 relies on the card number being even
     * you can't have your code blow up with unusual inputs
     *
     */

    for (int i = cardNumbers.length - 1; i >= 0; i -= 2) {
        sum += cardNumbers[i];
    }
    for (int i = cardNumbers.length - 2; i >= 0; i -= 2) {
        int dig = cardNumbers[i] * 2;
        while (dig > 0) {
            sum += dig % 10;
            dig /= 10;
        }
    }

    return sum;
}

Now you can do something like:

public void makeItValid() {
    int[] invalidNumbers = cardToNumbers(creditCardNumber);

    int sum = calcTotalSum(invalidNumbers);

    while ((sum = calcTotalSum(invalidNumbers)) % 10 != 0)
        invalidNumbers[invalidNumbers.length - 1]--;

    totalSum = sum;
    checkDigit = invalidNumbers[invalidNumbers.length - 1];
}

But you should be able to just subtract the difference to find the valid check digit:

if (totalSum % 10 != 0) checkDigit -= totalSum % 10;

Or something like:

public void makeItValid() {
    int[] invalidNumbers = cardToNumbers(creditCardNumber);

    checkDigit = invalidNumbers[invalidNumbers.length - 1] -= totalSum % 10;
    totalSum = calcTotalSum(invalidNumbers);

    valid = true;
}

Some asides,

I would recommend storing the digits as a field and have checkDigit represent an index in the array. This would simplify some of the operations you are doing.

I would also suggest not to be "silently" changing fields internally IE like in your makeItValid method unless this is a specification of the assignment. I think a better form is to let the "owning" code make the changes itself which is more clear externally. A somewhat complete implementation would look like this:

public class CreditCard {
    public static void main(String[] args) {
        if (args.length == 0) return;

        CreditCard card = new CreditCard(args[0]);

        if (!card.isValidNumber()) {
            card.setCheckDigit(card.getValidCheckDigit());
        }
    }

    private final String cardText;
    private final int[] cardDigits;
    private final int cdIndex;

    public CreditCard(String ct) {
        cardDigits = cardToNumbers(cardText = ct);

        if ((cdIndex = cardDigits.length - 1) < 0) {
            throw new IllegalArgumentException("# had no digits");
        }
    }

    public boolean isValidNumber() {
        return calcTotalSum(cardDigits) % 10 == 0;
    }

    public void setCheckDigit(int dig) {
        cardDigits[cdIndex] = dig;
    }

    public int getValidCheckDigit() {
        int sum = calcTotalSum(cardDigits);
        if (sum % 10 != 0) {
            return cardNumbers[cdIndex] - sum % 10;
        } else {
            return cardNumbers[cdIndex];
        }
    }

    // above static methods
}

The best form IMO would be to disallow creation of a credit card object at all unless the check digit is valid. As an OOP principle it should not make sense to create invalid credit cards. The constructor should throw an exception if the card is invalid and have a static method to correct the number.

I would do something like the following (shortened):

public class CreditCard {
    public CreditCard(String number) {
        if (!validateCheckDigit(number)) {
            throw new IllegalArgumentException("check digit failure");
        }
    }
}

public static void main(String[] args) {
    String number = args[0];
    CreditCard card = null;

    boolean valid = false;
    do {
        try {
            card = new CreditCard(number);
            valid = true;
        } catch (IllegalArgumentException e) {
            number = CreditCard.correctCheckDigit(number);
        }
    } while (!valid);
}

I guess that's more or less doing your homework for you but I'm sure you can learn from it.