CS50 pset1: credit card classification and checksum validation

Question

I wrote the following code for the cs50 pset1 credit and it passes all the checks. However, I think that it is too long (specially the last part) and would like to improve it. I would be very grateful, if someone could check my code and give me some feedback.

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


int main(void)
{
    //Ask user for the credit card number and store the input in a string
    string card_number;
    card_number = get_string("Number: ");

    // Store the string length
    int string_length = strlen(card_number);

    //Iterate through each character to check if it is a digit
    for (int character = 0; character < string_length; character++)
    {
        if (!isdigit(card_number[character]))
        {
            printf("INVALID\n");
            break;
        }
    }

    //Calculate the check_sum
    int check_sum = 0;

    for (int character = string_length - 1; character >= 0; character -= 2)
    {
        check_sum += card_number[character] - '0';
    }

    for (int character = string_length - 2; character >= 0; character -= 2)
    {
        int digit = ((card_number[character] - '0') * 2);

        while (digit > 0)
        {
            check_sum += (digit % 10);
            digit /= 10;
        }
    }

    //Print the credit card provider
    if (check_sum % 10 == 0 && string_length == 15 && card_number[0] == '3' && card_number[1] == '4')
    {
        printf("AMEX\n");
    }
    else if (check_sum % 10 == 0 && string_length == 15 && card_number[0] == '3' && card_number[1] == '7')
    {
        printf("AMEX\n");
    }
    else if (check_sum % 10 == 0 && string_length == 13 && card_number[0] == '4')
    {
        printf("VISA\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '4')
    {
        printf("VISA\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '1')
    {
        printf("MASTERCARD\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '2')
    {
        printf("MASTERCARD\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '3')
    {
        printf("MASTERCARD\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '4')
    {
        printf("MASTERCARD\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '5')
    {
        printf("MASTERCARD\n");
    }
    else
    {
        printf("INVALID\n");
    }

}

Answer 1

You should break the code up into functions, one of the functions should be to check if each character is a digit and that function should return a bool value that indicates success or failure. In the current code if there is an invalid character the loop stops when it finds an in valid character, but the program still performs the check for the company that issued the card. If the card number is not valid the program should not continue with the processing.

Testing of code should always test negative paths as well as positive tests. This means that the testing should test bad values as well as good values.

Use the Proper Types

The C library function strlen(char* str) return the type size_t, this is the largest unsigned integer value the computer you are using can use. It would be much better if string_length was declared as size_t.

Initialize Variables in the Declarations

In C the language doesn't provide a default initialization of the variable so variables should be initialized as part of the declaration. For readability and maintainability each variable should be declared and initialized on its own line.

   //Ask user for the credit card number and store the input in a string
    char* card_number = get_string("Number: ");

    // Store the string length
    size_t string_length = strlen(card_number);

    bool isValid = check_for_invalid_input(card_number, string_length);

Putting it All Together

#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#include <string.h>

static bool check_for_invalid_input(char* card_number, size_t len)
{
    //Iterate through each character to check if it is a digit
    for (int character = 0; character < len; character++)
    {
        if (!isdigit(card_number[character]))
        {
            printf("INVALID\n");
            return false;
        }
    }

    return true;
}

static int calculate_check_sum(char* card_number, size_t len)
{
    int check_sum = 0;

    for (int character = len - 1; character >= 0; character -= 2)
    {
        check_sum += card_number[character] - '0';
    }

    for (int character = len - 2; character >= 0; character -= 2)
    {
        int digit = ((card_number[character] - '0') * 2);

        while (digit > 0)
        {
            check_sum += (digit % 10);
            digit /= 10;
        }
    }

    return check_sum;
}

static void report_card_issuer(int check_sum, char* card_number, size_t string_length)
{
    if (check_sum % 10 == 0 && string_length == 15 && card_number[0] == '3' && card_number[1] == '4')
    {
        printf("AMEX\n");
    }
    else if (check_sum % 10 == 0 && string_length == 15 && card_number[0] == '3' && card_number[1] == '7')
    {
        printf("AMEX\n");
    }
    else if (check_sum % 10 == 0 && string_length == 13 && card_number[0] == '4')
    {
        printf("VISA\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '4')
    {
        printf("VISA\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '1')
    {
        printf("MASTERCARD\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '2')
    {
        printf("MASTERCARD\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '3')
    {
        printf("MASTERCARD\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '4')
    {
        printf("MASTERCARD\n");
    }
    else if (check_sum % 10 == 0 && string_length == 16 && card_number[0] == '5' && card_number[1] == '5')
    {
        printf("MASTERCARD\n");
    }
    else
    {
        printf("INVALID\n");
    }
}

int main(void)
{
    //Ask user for the credit card number and store the input in a string
    char* card_number = get_string("Number: ");

    // Store the string length
    size_t string_length = strlen(card_number);

    bool isValid = check_for_invalid_input(card_number, string_length);

    if (isValid)
    {
        int check_sum = calculate_check_sum(card_number, string_length);

        report_card_issuer(check_sum, card_number, string_length);
    }
    else
    {
        return EXIT_FAILURE;
    }

    return EXIT_SUCCESS;
}

Answer 2

Two more things about identifying the issuer
(pacmaninbw's report_card_issuer()/the last part that seems too long):

• There is a lot of common parts
  • in the conditions as well as
  • in the statements controlled
• it does two things:
  • identify the issuer
  • report in human readable form

Each and every condition starts with check_sum % 10 == 0:
handle this once and for all (maybe in check_for_invalid_input()).
In the conditions, don't repeat yourself:

static char const *
card_issuer(int check_sum, char const* card_number, size_t string_length)
{
    if (check_sum % 10 != 0) {
        return "INVALID";
    }
    if (string_length == 15 && card_number[0] == '3'
        && (card_number[1] == '4' || card_number[1] == '7')) {
        return "AMEX";
    }
    if ((string_length == 13 || string_length == 16)
        && card_number[0] == '4') {
        return "VISA";
    }
    if (string_length == 16 && card_number[0] == '5'
        && (card_number[1] == '1'
            || card_number[1] == '2'
            || card_number[1] == '3'
            || card_number[1] == '4'
            || card_number[1] == '5')) {
        return "MASTERCARD";
    }

    return "INVALID";
}

(- oh well, the last condition could be less repetitive still using
'1' <= card_number[1] && card_number[1] <= '5' or
NULL != strchr("12345", card_number[1]),
or shorter introducing int const digit0 = card_number[0], digit1 = card_number[1];.)

The main point here is
Be sure to re-test after such a change.

Answer 3

Avoid wasting time with hostile input

Whenever there is a upper length bound to string validation, to efficiently guard against hostile input like "01234567890...(millions of digits)", use memchr(s, 0, max_size) instead of strlen(s). After all, validation needs to deal with all sorts of not-so-nice or errant input coming from a user.

memchr() has the advantage over strlen() in that there is a small-ish upper iteration count bound, like 17, here.

#define CC_LEN_MIN 13
#define CC_LEN_MAX 16

char *nul_addr = memchr(card_number, '\0', CC_LEN_MAX + 1);
if (nul_addr == NULL || (nul_addr - card_number) < CC_LEN_MIN) {
  return "INVALID"; // Too big or too small
}

cc_length = nul_addr - card_number;
...

is...(ch) trouble when ch < 0

is...() functions only defined for unsigned char values and EOF. Access the string via unsigned char * to avoid negative values when char is sometimes signed. This also properly handles signed non-2's complement encoding with -0 - likely no longer an issue in upcoming C2X.

To test if a string is only digits:

const unsigned char *digit = (const unsigned char *) card_number;
while (isdigit(*digit)) {
  digit++;
}
// Success if code stopped due to a null character.
bool success = *digit == '\0';

---

I think that it is too long (specially the last part) and would like to improve it.

Simplified CC type determination

By this time, we already know card_number[] is all digits and of a reasonable length.

// Test check code
if (check_sum % 10) {
  return "Invalid";
}

Then consider a table driven set of rules. Identification cards and Anatomy of a Credit Card provide extended information. Then its easy to add/maintain other cards like Discover, Diners Club, less known Dankort, ...

typedef enum {
  ccn_none, ccn_amex, ccn_dclb, ccn_dkrt, ccn_disc, ccn_mcrd, ccn_visa, ccn_N
} credit_card_network;

static credit_card_network ccn_match(const char *ccn, size_t length) {
  //https://en.wikipedia.org/wiki/Payment_card_number#Issuer_identification_number_(IIN)
  const struct {
    credit_card_network network;
    size_t number_length;
    unsigned iin_length; // Issuer identification number
    int iin_min;
    int iin_max;
  } credit_card_rule[] = { //
      {ccn_amex, 15, 2, 34, 34}, // AMEX
      {ccn_amex, 15, 2, 37, 37}, //
      {ccn_dclb, 14, 2, 36, 36}, // Diners Club International
      {ccn_dkrt, 16, 4, 5019, 5019}, // Dankort
      {ccn_mcrd, 16, 2, 51, 55}, // Mastercard
      {ccn_mcrd, 16, 4, 2221, 2720}, // Mastercard
      {ccn_visa, 13, 1, 4, 4}, // Visa
      {ccn_visa, 16, 1, 4, 4}, // Visa
  };
  size_t n = sizeof credit_card_rule / sizeof credit_card_rule[0];

  for (size_t i = 0; i < n; i++) {
    if (credit_card_rule[i].number_length != length) {
      continue;
    }
    char prefix[12];
    memcpy(prefix, ccn, credit_card_rule[i].iin_length);
    prefix[credit_card_rule[i].iin_length] = '\0';
    int iin = atoi(prefix);
    if (iin >= credit_card_rule[i].iin_min
        && iin < credit_card_rule[i].iin_max) {
      return credit_card_rule[i].network;
    }
  }
  return ccn_none;
}

Answer 4

This builds on greybeard's answer, which in turn builds on pacmaninbw's.

I would recommend combining the test for non-numeric input and the checksum verification into a single function (as this is simply validating the syntactical form of the number), and the part that checks the issuer be another (this is the function that must be updated when new issuers appear).

Perhaps the checksumming function could remove spaces and - characters that are often inserted into credit-card numbers, to make it more user-friendly?

The card-issuer test looks like a perfect candidate for a switch, since the first digit determines which issuer we should be validating:

static char const *
card_issuer(char const* card_number, size_t string_length)
{
    switch (card_number[0]) {
    case '3':
        /* AmEx */
        if (string_length != 15) { break; }
        if (card_number[1] != '4' && card_number[1] != '7') { break; }
        return "AMEX";
    case '4':
        /* VISA */
        if (string_length != 13 && string_length != 16) { break; }
        return "VISA";
    case '5':
        /* MasterCard */
        if (string_length != 16) { break; }
        if (card_number[1] < '1' || card_number[1] > '5') { break; }
        return "MASTERCARD";
    }

    return "INVALID";
}

I think this version is a little clearer, as it shows very obviously that we use the first digit to decide which other validation rules to apply.