Credit card verification: string conversion most optimal?

Question

Below is some code which verifies a credit card number using the checksum as well as check if number of digits are appropriate as well if digits start with right numbers. I am not sure if converting the double into a string was the best bet. I wasn't going to at first but had trouble figuring out the modulo math to get every second digit without knowing the length of the double (# of digits).

Also, in my use of strtok(), what should I be doing with the balance of the string after the delimiter? Is that hanging out in memory somewhere?

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

int main(void)
{
    double cardnumber;
    printf("Give me a number: \n");
    scanf("%lf", &cardnumber);

    if(cardnumber < 1000000000000 || cardnumber > 10000000000000000)
    {
        printf("INVALID\n");
        return 0;
    }
    if(cardnumber < 100000000000000 && cardnumber > 9999999999999)
    {
        printf("INVALID\n");
        return 0;   
    }

    char creditcard[17];
    sprintf(creditcard, "%f", cardnumber);
    char* ptr_cc;
    ptr_cc = strtok(creditcard,".");
    int card_size = strlen(ptr_cc);
    int sum = 0;
    for(int i = 1; i < card_size; i+=2)
    {
        int x = creditcard[card_size-1-i] - '0';
        int prod = 2 * x;
        if(prod>=10)
        {
            prod = prod%10 + prod/prod%10;
        }
        sum += prod;      
    }
    for(int i = 0; i < card_size; i+=2)
    {
        int x = creditcard[card_size-1-i] - '0';
        sum += x;      
    }
    if(sum%10 != 0)
    {
        printf("INVALID\n");
        return 0; 
    }
    else if(creditcard[0] == '4')
    {
        printf("VISA\n");
        return 0; 
    }
    else if(creditcard[0] == '3' && (creditcard[1] == '7' || creditcard[1] =='4'))
    {
        printf("AMEX\n");
        return 0; 
    }
    else if(creditcard[0] == '5' && (creditcard[1] >='1' && creditcard[1] <='5'))
    {
        printf("MASTERCARD\n");
        return 0; 
    }
    else
    {
        printf("INVALID\n");
        return 0;    
    }         
}

Answer 1

As @Keith says, credit card numbers should be treated as strings, not numbers, and definitely not floating-point numbers. If you want to ensure that the input contains only digits (and maybe spaces), use strspn(). Squeeze out any spaces, then validate the length using strlen().

The Luhn checksum check should be in its own function. The loop indexes would be more natural counting down, I think, since you are taking every other digit starting from the right.

int is_valid_luhn(const char *creditcard)
{
    int card_size = strlen(creditcard);
    int sum = 0;
    for(int i = card_size - 2; i >= 0; i -= 2)
    {
        int digit = creditcard[i] - '0';
        int prod = 2 * digit;
        sum += prod / 10 + prod % 10;      /* No special case needed */
    }
    for(int i = card_size - 1; i >= 0; i -= 2)
    {
        sum += creditcard[i] - '0';
    }
    return sum % 10 == 0;
}

Answer 2

Things you did well

• Readability - everything was well organized and spaced neatly.

• Initializing variables within your for loops/adhering to the C99 standard (which is the minimum you should abide by in my opinion).

• Marking your function arguments as void when you don't take in any parameters.

• Use of double instead of float (even though you should be using a string to represent the credit card number).

• Implementation of the Luhn algorithm, even though it isn't the most elegant. @200_success covered that in depth (+1 to that answer), I won't go into it.

Things you could improve

Bugs

• As already noted, you should be using strings to hold the credit card numbers, not integers, and especially not floating point numbers. But I won't completely re-iterate what has been already mentioned.

• Right now you are using the float format (%f) to compose a string.

  sprintf(creditcard, "%f", cardnumber);
  

  This didn't work for me when I ran the program on my system, and caused a error to be thrown during runtime. Since cardnumber was declared as a double, you should use the general format instead, which can handle both float and double types.

  sprintf(creditcard, "%g", cardnumber);
  

Variables/Initialization

• card_size will lose integer precision because of the implicit conversion from an unsigned long to int.

  int card_size = strlen(ptr_cc);
  

  You could either manually cast the return value from strlen() to an int, or you could simply declare the type of card_size to be unsigned long or size_t.

  size_t card_size = strlen(ptr_cc)
  

Standards

• strtok is limited to tokenizing only one string (with one set of delimiters) at a time, and it can't be used while threading. Therefore, strtok is considered deprecated.

  Instead, use strtok_r or strtok_s which are threading-friendly versions of strtok. The POSIX standard provided strtok_r, and the C11 standard provides strtok_s. The use of either is a little awkward, because the first call is different from the subsequent calls.

  1. The first time you call the function, send in the string to be parsed as the first argument.

  2. On subsequent calls, send in NULL as the first argument.

  3. The last argument is the scratch string. You don't have to initialize it on first use; on subsequent uses it will hold the string as it is parsed so far.

  To demonstrate its use, I've written a simple line counter (of only non-blank lines) using the POSIX standard one. I'll leave the choice of what version to use and implementation into your program up to you.

  #include <string.h> // strtok_r
  
  int countLines(char* instring)
  {
      int counter = 0;
      char *scratch, *txt;
      char *delimiter = "\n";
      for (; (txt = strtok_r((!counter ? instring : NULL), delimiter, &scratch)); counter++);
      return counter;
  }
  

  If you implement this, you have your question answered.

  Also, in my use of strtok(), what should I be doing with the balance of the string after the delimiter? Is that hanging out in memory somewhere?

  It's not hanging out in memory, it's in the scratch string you provided.

• You don't have to return 0 at the end of main(), just like you wouldn't bother putting return; at the end of a void-returning function. The C standard knows how frequently this is used, and lets you not bother.

  C99 & C11 §5.1.2.2(3)

  ...reaching the } that terminates the main() function returns a value of 0.

Syntax/Styling

• You don't have to return inside every conditional test.

  if(sum%10 != 0)
  {
      printf("INVALID\n");
      return 0; 
  }
  else if(creditcard[0] == '4')
  {
      printf("VISA\n");
      return 0; 
  }
  else if(creditcard[0] == '3' && (creditcard[1] == '7' || creditcard[1] =='4'))
  {
      printf("AMEX\n");
      return 0; 
  }
  else if(creditcard[0] == '5' && (creditcard[1] >='1' && creditcard[1] <='5'))
  {
      printf("MASTERCARD\n");
      return 0; 
  }
  else
  {
      printf("INVALID\n");
      return 0;    
  }
  

  You can pull out all of those return 0;'s out to the end of the entire test block, so you have the return in one place. But returning 0 is unnecessary, as seen in my previous points.

• You don't return unique identifiers when you encounter an error.

  if(cardnumber < 100000000000000 && cardnumber > 9999999999999)
  {
      printf("INVALID\n");
      return 0;   
  }
  

  Using unique identifiers will aid you a lot in debugging, because it can help you pinpoint where something went wrong. Typically, positive numbers are used to indicate errors.

  if(cardnumber < 100000000000000 && cardnumber > 9999999999999)
  {
      printf("INVALID\n");
      return 1;   
  }
  

• Use puts() instead of printf() when you aren't formatting a string.

  puts("INVALID");
  

• Prefer snprintf() to sprintf().

Answer 3

The other answers offer great advice, but I have some things to add pertaining to naming:

• This doesn't sound like a good input prompt:

  printf("Give me a number: \n");
  

  Imagine, for a second, that the user briefly forgot what this program is for. If they see this prompt, they'll just input any number. Even with input validation, you should make the process as clear as possible for the user.

  Instead, you should ask specifically for a credit card number. Also consider using something more formal than "give me" at the start.

  puts("Input a credit card number: \n");
  

  (@syb0rg's advice regarding puts() also applies here.)

• Your variable naming is inconsistent:

  double cardnumber;
  

  int card_size;
  

  The latter variable is an example of "snake_case" naming, which is an acceptable form of naming (the alternative being "camelCase").

  Since the former variable consists of two words, consider using the same naming convention with that and similar variables of the same form:

  double card_number;
  

• This variable sounds very generic:

  int sum = 0;
  

  Sum of what, exactly? Even if you're only summing one thing here, you should still name it based on what it's summing. It'll also be beneficial if you end up needing another sum-type variable.

  The same also applies to prod, which should at least be spelled out completely.

• Try to avoid single-character variable names (except for simple loop counters):

  int x;
  

  Unless the context is obvious, the variable name should adequately convey its meaning, enough to not need a comment to do the same. This also helps with maintenance and prevents the reader, or even yourself, from having to remember its meaning at any later point in the code.