Validation of credit card number

Question

I have written the code for credit card number validation and it works correctly but I feel that the code could be reduce to fewer lines.

#include <stdio.h>

int main()

{

    long long int  no;

    do
    {
        printf("Card number: ");
        scanf("%lld", &no);
    }

    while(no<10);

    int d_1,d_2,d_3,d_4,d_5,d_6,d_7,d_8,d_9,d_10,d_11,d_12,d_13,d_14,d_15,d_16;

    d_16 = no%10;
    d_15 = ((no%100)/10)*2;
    d_14 = (no%1000)/100;
    d_13 = ((no%10000)/1000)*2;
    d_12 = (no%100000)/10000;
    d_11 = ((no%1000000)/100000)*2;
    d_10 = (no%10000000)/1000000;
    d_9 = ((no%100000000)/10000000)*2;
    d_8 = (no%1000000000)/100000000;
    d_7 = ((no%10000000000)/1000000000)*2;
    d_6 = (no%100000000000)/10000000000;
    d_5 = ((no%1000000000000)/100000000000)*2;
    d_4 = (no%10000000000000)/1000000000000;
    d_3 = ((no%100000000000000)/10000000000000)*2;
    d_2 = (no%1000000000000000)/100000000000000;
    d_1 = ((no%10000000000000000)/1000000000000000)*2;

    int od[] = {d_1,d_3,d_5,d_7,d_9,d_11,d_13,d_15};
    int ed[] = {d_2,d_4,d_6,d_8,d_10,d_12,d_14,d_16};
    int i,add=0;

    for (i=1;i<=8;i++)
    {
         if(od[i] >= 10)
         {
             int nod = (od[i]%10)+(od[i]/10);
             od[i] = nod;
         }
    }
    for (int j=0;j<8;j++)
    {
        add = add + od[j];
    }
    for (i=1;i<=8;i++)
    {
        if(ed[i] >= 10)
        {
            int ood = (ed[i]%10)+(ed[i]/10);
            ed[i] = ood;
        }
    }
    for (int j=0;j<8;j++)
    {
        add = add + ed[j];
    }

    if ((add%10)==0)
    {
        printf("The card is valid");
    }
    else
    {
    printf("The card is invalid");
    }

}

Answer 1

I don't like this:

do
{
    printf("Card number: ");
    scanf("%lld", &no);
}

while(no<10);

That blank line makes it look like the while (); is a separate (possibly infinite) loop. I recommend writing the while keyword straight after the closing brace, like this:

do {
    printf("Card number: ");
} while (scanf("%lld", &no) != 1);

(I've also changed the test so that we don't access an uninitialised no if the scanf() fails - it's still flawed, though, as any permanent stdin failure will lead to an infinite loop).

---

It's not clear why we have d_1, d_2, ..., etc. when all we do with them is to copy them into array members - why not just assign to the array members directly?

---

Instead of dividing no by successively bigger numbers each time, we could modify it as we go, like this:

d_16 = no % 10;
no /= 10;
d_15 = no % 10 * 2;
no /= 10;
d_14 = no % 10;

It's starting to become clear how to transform this into a loop. The missing part is whether to multiply by 2 at each step; we can tell whether the loop index is even or odd, by considering it modulo 2.

Once we are considering the digits in a loop, we can perform the addition as we go, meaning that we no longer need all those local variables.

Answer 2

d_16 = no%10;
d_15 = ((no%100)/10)*2;
d_14 = (no%1000)/100;
d_13 = ((no%10000)/1000)*2;
d_12 = (no%100000)/10000;
d_11 = ((no%1000000)/100000)*2;
d_10 = (no%10000000)/1000000;
d_9 = ((no%100000000)/10000000)*2;
d_8 = (no%1000000000)/100000000;
d_7 = ((no%10000000000)/1000000000)*2;
d_6 = (no%100000000000)/10000000000;
d_5 = ((no%1000000000000)/100000000000)*2;
d_4 = (no%10000000000000)/1000000000000;
d__3 = ((no%100000000000000)/10000000000000)*2;
d_2 = (no%1000000000000000)/100000000000000;
d_1 = ((no%10000000000000000)/1000000000000000)*2;

You're using too many magic numbers in your program, and they are prone to error. Instead, you could use a loop for this calculation.