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

• dfhwze: Could you provide the specification of a valid credit card number and some unit tests?
• dfhwze: Did you make a typo d3 has 2 different variable names?

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.