Determining whether a provided credit card number is valid according to Luhn’s algorithm

Question

I have been working on credit problem for edx CS50 and got it working as intended. an issue I find with it is the way I have to reinitialise the cardNumber variable each time it enters a loop, and there are plenty of loops in my solution. Same goes for my bool value. Is there a better, cleaner way to go around this?

#include <stdio.h>
#include <cs50.h>

// Implement a program that determines whether a provided credit card number
// is valid according to Luhn’s algorithm.

int main(void)
{
    long long userInput;

    // Prompt for user input as long long.
    do
    {
        userInput = get_long_long("Enter credit card number to validate: ");

        // Ensure user inputs integer otherwise prompt "Retry: "
        if (userInput < 0)
        {
            printf("Retry: \n");
        }
    } while (userInput < 0);

    long long cardNumber = userInput;
    int checkSum = 0;
    int secDig, lastDig, multDig;

    // start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
    while (cardNumber > 0)
    {
        // sum digits that weren't multiplied.
        lastDig = (cardNumber % 10);
        // add the sum to the sum of the digits that weren't multipied by 2.
        checkSum += lastDig;

        secDig = (cardNumber / 10) % 10;
        cardNumber /= 100;

        // multiply every other digit
        multDig = secDig * 2;

        // add those products (separate /10, return modulus of 10) digits together.
        if (multDig > 10)
        {
            checkSum += (multDig % 10) + 1;
        }
        else if (multDig == 10)
        {
            checkSum += 1;
        }
        else
        {
            checkSum += multDig;
        }
    }

    // validate checksum
    // if sum of digits ends in 0 ergo % 10 = 0 then number is valid.
    bool valid = false;
    if (checkSum % 10 == 0)
    {
        valid = true;
    }

    int numOfDigits = 0;
    cardNumber = userInput;

    // validate number's length
    // track number's length
    while (cardNumber > 0)
    {
        cardNumber /= 10;
        numOfDigits++;
    }

    cardNumber = userInput;

    // validate company's identifier
    // if 15 digits, starts with 34 or 37 - AMEX

    if (valid == true && numOfDigits > 12)
    {
        if (numOfDigits == 15)
        {
            for (int i = 0; i < numOfDigits - 2; i++)
            {
                cardNumber /= 10;
            }
            if (cardNumber == 34 || cardNumber == 37)
            {
                printf("AMEX\n");
                valid = true;
            }
            else
            {
                valid = false;
            }
        }
        // if 16 digits, starts with 51-55 - MASTERCARD
        cardNumber = userInput;
        if (numOfDigits == 16)
        {
            for (int i = 0; i < numOfDigits - 2; i++)
            {
                cardNumber /= 10;
            }
            if (cardNumber >= 51 && cardNumber <= 55)
            {
                printf("MASTERCARD\n");
                valid = true;
            }
            else
            {
                valid = false;
            }
        }
        // if 13 OR 16 digits, starts with 4 - VISA
        cardNumber = userInput;
        if (numOfDigits == 13 || numOfDigits == 16)
        {
            for (int i = 0; i < numOfDigits - 1; i++)
            {
                cardNumber /= 10;
            }
            if (cardNumber == 4)
            {
                printf("VISA\n");
                valid = true;
            }
            else
            {
                valid = false;
            }
        }
        if (valid == false)
        {
            printf("INVALID\n");
        }
    }
    else
    {
        printf("INVALID\n");
    }
}

All comments are part of pseudocode for this solution. I left them there to satisfy style50. I would appreciate advice so I can write cleaner next time and make it into a habit. I am a beginner starting CS degree next year with only experience with doing C# courses with Mosh Hamedani.

Answer 1

First thing I think of is that you have a pretty long main. You should split the code in more functions. Your code could look like this:

long long readCardnumber();
bool validateChecksum(long long cardnumber);
typedef enum { invalid, amex, mastercard, visa } company;
company getCompany(long long cardnumber);


int main() 
{
    long long cardnumber = readCardnumber();
    bool valid = validateCardnumber(cardnumber);
    if(!valid) {
        printf("INVALID\n");
        exit(0);
    }
    company comp = getCompany(long long cardnumber);
    switch(comp) {
        case visa: printf("VISA\n"); break;
        case mastercard: printf("MASTERCARD\n"); break;
        case amex: printf("AMEX\n"); break;
        default: printf("INVALID\n");
    }
}

Another thing is that you are writing if (valid == false). Just write if (!valid) instead. At least I think it looks way better, since it feels natural to read it as "if not valid". Also, write if (valid) instead of if (valid == true).

One general advice on boolean expressions. Consider writing if(3 == var) instead of if(var == 3). The reason is if you accidentally write = instead of == you will get compiling code with an annoying bug if you write the constant on the right. If you have it on the left, you will instead get a compilation error, which is much nicer to handle. (I don't do this myself though, and the only reason is that I don't like how it looks.)

There are some other stuff that I would consider bad design, like when you are resetting the value of cardNumber several times. However, this is a direct consequence of not having separate functions.

Answer 2

This is a pretty good start. Your code is readable and understandable. Your variable names are descriptive, and you've used the right types for the work you're doing. Here are a few suggestions for improvements.

Dealing With Strings vs. Numbers

This is a case where some of your code would be easier to write if you were dealing with strings instead of numbers. You're fortunate in that a 13-16 digit number fits inside of a long long, and there should be no valid credit card numbers with leading 0s. That gets rid of 2 problematic issues with validating numerical inputs.

If you obtained the user's input as a string instead of a long long, you'd need to convert the characters to values so you could check the checksum. At first that seems like more work. But here's a function that converts a char to an int:

int charToInt(const char c)
{
    if (isdigit(c))
    {
            return (int)(c - '0');
    }
    return -1;
}

Once you have that, getting the digits out of the card is about as easy as doing it numerically, but you don't have to keep dividing the number and making copies of it. Your checksum loop ends up looking like this:

char* cardNumber;  // Assume this was gotten above and is filled out correctly

// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
int nextDigit;
unsigned long numberLen = strlen(cardNumber);
char* nextDigitChar = cardNumber + numberLen - 1;
while (nextDigitChar > cardNumber)
{
    // sum digits that weren't multiplied.
    nextDigit = charToInt(*nextDigitChar);
    
    // add the sum to the sum of the digits that weren't multipied by 2.
    checkSum += nextDigit;
    nextDigitChar--;
    
    // Get the security digit
    secDig = charToInt(*nextDigitChar);
    nextDigitChar--;
    
    // multiply every other digit
    multDig = secDig * 2;
    
    // add those products (separate /10, return modulus of 10) digits together.
    if (multDig > 10)
    {
        checkSum += (multDig % 10) + 1;
    }
    else if (multDig == 10)
    {
        checkSum += 1;
    }
    else
    {
        checkSum += multDig;
    }
}

Now this probably doesn't look like a whole lot less work than what you were doing before. However, notice that we haven't modified the input at all. So we don't need to copy it again to do other work. Speaking of which...

I would check the length of the input before doing any other work. If it isn't 13-16 characters, then there's no point in doing any checksum because we already know it's invalid. Note also, that this makes the last part of your program much easier, too. You can get the first 2 digits by simply looking at the string.

if (valid)
{
    int vendorCode = charToInt(cardNumber[0]) * 10 + charToInt(cardNumber[1]);
    if ((numberLen == 15) && (vendorCode >= 34 && vendorCode <= 37))
    {
        printf("AMEX\n");
    }
    else if ((numberLen == 16) && (vendorCode >= 51 && vendorCode <= 55))
    {
        printf("MASTERCARD\n");
    }
    else if (((numberLen == 13) || (numberLen == 16)) && (vendorCode / 10 == 4))
    {
        printf("VISA\n");
    }
    else
    {
        valid = false;
    }
}

if (!valid)
{
    printf("INVALID\n");
}