6
\$\begingroup\$

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");
    }

}
\$\endgroup\$

4 Answers 4

6
\$\begingroup\$

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;
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ check_for_invalid_input() is a confusing name - does a true value mean it's valid or invalid? Something like number_is_valid() would be clearer. And I think it (and calculate_check_sum()) needs to actually return a value - or was that intended to be left as an exercise? \$\endgroup\$ Aug 22, 2022 at 16:24
  • \$\begingroup\$ @TobySpeight Fixed calcuylated_check_sum(). Won't argue about check_for_invalid_input(). \$\endgroup\$
    – pacmaninbw
    Aug 22, 2022 at 17:49
  • \$\begingroup\$ "this is the largest unsigned integer value the computer you are using can use." --> Not quite. uintmax_t may be wider and is somewhat processor width related. size_t is the size needed for sizing and array indexing. Its width is memory model dependent. "much better if string_length was declared as size_t" remains true, though. \$\endgroup\$ Aug 22, 2022 at 18:33
  • 1
    \$\begingroup\$ @chux-ReinstateMonica Thanks, corrected. \$\endgroup\$
    – pacmaninbw
    Aug 22, 2022 at 19:05
8
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Depending on how future code might want to use the results, it might make sense to have card_issuer return an enum value instead, and then provide a function to convert that enum value into a human-readable string. \$\endgroup\$ Aug 22, 2022 at 21:38
  • \$\begingroup\$ @DanielSchepler Quite right, a char const *card_issuer() still does one and a half thing: In addition to identifying some issuers, it returns a representation immediately suitable for users understanding English. I just wanted to directly address the last part seems too long (& testing program changes) - I even skipped riding my favourite hobby horse of never "publishing" code not containing a specification what it is to accomplish. \$\endgroup\$
    – greybeard
    Aug 22, 2022 at 22:47
7
\$\begingroup\$

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;
}
\$\endgroup\$
4
  • \$\begingroup\$ It looks like iin_length can be derived from iin_min or iin_max. Or we could pass those as strings and use strcmp() instead of <. \$\endgroup\$ Aug 23, 2022 at 9:30
  • \$\begingroup\$ @TobySpeight True about alternative code could derive iin_length from iin_min and iin_max in many cases. I am not certain it would work well with all from the table. Alt code could also use a range for number_length. The larger point being that this section of code should be table driven for clarity, flexibility and maintenance. \$\endgroup\$ Aug 23, 2022 at 11:28
  • 1
    \$\begingroup\$ @TobySpeight On further review, I might re-write code to accept the table's syntax. e. g.: {"Discover Card", "6011, 644-649, 65", "16-19", "Luhn"} and prior to ccn_match() perform a one-time translation to an easier to use struct. The overall goal: making table updates easy. , \$\endgroup\$ Aug 23, 2022 at 11:47
  • \$\begingroup\$ Oh yes, the table-driven approach is far clearer than any other. Thanks for showing how to do that well! \$\endgroup\$ Aug 23, 2022 at 14:59
3
\$\begingroup\$

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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.