Checking the type and validity of a credit card with Luhn's algorithm in C (CS50)

Question

I'm taking CS50 Intro to CS. In this assignment, we're asked to make a program that can identify what type of credit card (AMEX, Visa, MasterCard) is given in the input (and whether it is valid.) Validity is checked using Luhn's Algorithm. Here's how the algorithm works:

1. Multiply every other digit by 2, starting with the number’s second-to-last digit, and then add those products’ digits together. (We're not adding the products together, rather the digits of the products!)
2. Add the sum to the sum of the digits that weren’t multiplied by 2.
3. If the total’s last digit is 0, the number is valid!

Then, the type of credit card is checked by checking the first few digits of the credit card. I.e American Express numbers start with 34 or 37, VISA starts with 4, etc etc. Here's my implementation:

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

bool check_is_card_valid(long card);
int checksum(long card);
bool array_consists(const unsigned int arr[], int val, int arrlen);
string get_card_type(long card);
int get_nth_digit(long card, int digit);
int get_card_length(long card);

const unsigned int accepted_card_length[] = 
{
    15, // AMEX uses 15 digit numbers
    16, // MasterCard and VISA uses 16 digit numbers
    13  // VISA uses 13 digit numbers
};
const size_t acc_card_len = sizeof(accepted_card_length) / sizeof(*accepted_card_length);
const unsigned int AMEX_first_digits[] = {34, 37};
const size_t amex_digits_len = sizeof(AMEX_first_digits) / sizeof(*AMEX_first_digits);
const unsigned int MASTERCARD_first_digits[] = {51, 52, 53, 54, 55};
const size_t mc_digits_len = sizeof(MASTERCARD_first_digits) / sizeof(*MASTERCARD_first_digits);
const unsigned int VISA_first_digit = 4;


int main(void)
{
    long card_number = get_long("Card number: ");
    if (check_is_card_valid(card_number) == true)
    {
        printf("%s\n", get_card_type(card_number));
    } 
    else
    {
        printf("INVALID\n");
    }
}

/*
    Luhn's algorithm. Starting from the right, multiply every other digit by 2 and add up the digits
    add this sum to the sum of the digits that were not multiplied by 2. 
*/

int checksum(long card)
{
    int checksum = 0;
    for (int i = 0; i < get_card_length(card); i++)
    {
        if (i % 2 == 0)
        {
            checksum += get_nth_digit(card, i);
        }
        else if (i % 2 == 1)
        {
            int num = get_nth_digit(card, i) * 2;
            if (num > 9)
            {
                num = get_nth_digit(num, 0) + get_nth_digit(num, 1);
            }
            checksum += num;
        }
    }
    return checksum;
}

bool array_consists(const unsigned int arr[], int val, int arrlen)
{
    for (int i = 0; i < arrlen; i++)
    {
        if (val == arr[i])
        {
            return true;
        }
    }
    return false;
}

// Luhn's algorithm states that if the last digit of the checksum is 0, it is valid
bool check_is_card_valid(long card)
{
    if (array_consists(accepted_card_length, get_card_length(card), acc_card_len) == true)
    {
        return checksum(card) % 10 == 0;
    }
    else
    {
        return false;
    }
}

string get_card_type(long card)
{
    int card_length = get_card_length(card);
    int first_digit = get_nth_digit(card, card_length - 1);
    int second_digit = get_nth_digit(card, card_length - 2);
    int first_two_digits = (first_digit * 10) + second_digit;
    if (first_digit == VISA_first_digit)
    {
        return "VISA";
    }
    else if (array_consists(AMEX_first_digits, first_two_digits, amex_digits_len) == true)
    {
        return "AMEX";
    }
    else if (array_consists(MASTERCARD_first_digits, first_two_digits, mc_digits_len) == true)
    {
        return "MASTERCARD";
    }
    else 
    {
        return "INVALID";
    }

}

// gets nth digit from the right, starting with 0, using modulo
int get_nth_digit(long card, int digit)
{
    return fmod(floor(card / pow(10, digit)), 10);
}

int get_card_length(long card)
{
    return floor(log10(card)) + 1;
}

I would like some feedback on my code and how I could improve it; things like style, readability, cleanness, efficiency, etc. Is my code easily understandable? What about the comments - is there not enough comments? I try not to comment on my code when the function of something seems obvious. How do I improve the style, and are there any code conventions I'm not following? Can I optimize this to make it a faster process? Or maybe there's a better way to do the same thing? Basically, how do I improve my code?

Answer 1

Be very wary of using floating-point for exact integer math. If it isn't wrong (proving it right for all cases is often complicated and error-prone), it is at least inefficient.

Next, if you must process all digits of a number in order, producing them in order is simple and efficient. Calling code to isolate random digits on demand is inefficient and the cumbersome.

If you use a signed type for non-negative values, you have to verify the input is non-negative. Better fix the type.

If you put the definition before the first call-site, you don't need forward-declarations.

If a function or global is local to the current file, mark it static to enforce it. That simplifies / enables further optimizations due to inlining and avoids inadvertent symbol-clashes.

static int checksum(unsigned long n) {
    int r = 0;
    while (n) {
        r += n % 10;
        n /= 10;
        r += 2 * (n % 10);
        if (n % 10 > 4) r -= 9;
        n /= 10;
    }
    return r;
}

Answer 2

A simplification would be the following.

int checksum(long card)
{
    int checksum = 0;
    for (int i = 0; i < get_card_length(card); i++)
    {
        int digit = get_nth_digit(card, i);
        if (i % 2 == 0)
        {
            checksum += digit;
        }
        else
        {
            int num = digit * 2;
            if (num > 9) // 10, 12, 14, 16, 18
            {
                //num = get_nth_digit(num, 0) + get_nth_digit(num, 1);
                //num = 1 + num - 10;
                num -= 9;
            }
            checksum += num;
        }
    }
    return checksum;
}

• More readable by introducing digit.
• else if can become an else. As the values of % operands are non-negative, the result cannot be -1.
• get_nth_digit is a kludge, and in fact the final result seems to represent much more simple semantics.

The API to check a credit card number uses an intermediate checksum. It "last digit", value % 10, should be 0 for a valid checksum. So the function checksum could be a local "private" function. It is not entirely evident what you put in a .h and what in a .c.