# Thread: Could someone critique this for me, and suggest shorter alternatives?

1. ## Could someone critique this for me, and suggest shorter alternatives?

Hi there, just written a Luhn's algo programme. I have no previous experience in C (except for last couple of weeks) so I'm aware that my code will be unwieldy and quite simple. How could I have made this better?

Is there any way I could have grouped the letter int variable, for example, something like:

Code:
`int a-p = 0;`
Code:
```#include <stdio.h>
#include <math.h>

int main(void)
{
long ccnum = 0;
long AMEX;
long VISA;
long MASTERCARD;
int a, b, c, d, e, f, g, h, i, j, k, l, m, n, o, p = 0;
int a1, a2, b1, b2, c1, c2, d1, d2, e1, e2, f1, f2, g1, g2, h1, h2 = 0;
int cksum;

do
{
ccnum = get_long("Enter Number:");

if (ccnum <= 4000000000000 || ccnum >= 5999999999999999)
{
printf("INVALID\n");
return 0;
}

if (ccnum >= 300000000000000 && ccnum <= 339999999999999)
{
printf("INVALID\n");
return 0;
}

if (ccnum >= 300000000000000 && ccnum <= 369999999999999)
{
printf("INVALID\n");
return 0;
}

if (ccnum >= 5000000000000000 && ccnum <= 5099999999999999)
{
printf("INVALID\n");
return 0;
}

if (ccnum >= 5600000000000000)
{
printf("INVALID\n");
return 0;
}

}
while (ccnum < 4000000000000 || ccnum > 5999999999999999);

a = ccnum % 100 / 10 * 2;
b = ccnum % 10000 / 1000 * 2;
c = ccnum % 1000000 / 100000 * 2;
d = ccnum % 100000000 / 10000000 * 2;
e = ccnum % 10000000000 / 1000000000 * 2;
f = ccnum % 1000000000000 / 100000000000 * 2;
g = ccnum % 100000000000000 / 10000000000000 * 2;
h = ccnum % 10000000000000000 / 1000000000000000 * 2;

i = ccnum % 10;
j = ccnum % 1000 / 100;
k = ccnum % 100000 / 10000;
l = ccnum % 10000000 / 1000000;
m = ccnum % 1000000000 / 100000000;
n = ccnum % 100000000000 / 10000000000;
o = ccnum % 10000000000000 / 1000000000000;
p = ccnum % 1000000000000000 / 100000000000000;

if (a >= 10)
{
a1 = a % 10;
a2 = a / 10;
a = a1 + a2;
}

if (b >= 10)
{
b1 = b % 10;
b2 = b / 10;
b = b1 + b2;
}

if (c >= 10)
{
c1 = c % 10;
c2 = c / 10;
c = c1 + c2;
}

if (d >= 10)
{
d1 = d % 10;
d2 = d / 10;
d = d1 + d2;

}

if (e >= 10)
{
e1 = e % 10;
e2 = e / 10;
e = e1 + e2;
}

if (f >= 10)
{
f1 = f % 10;
f2 = f / 10;
f = f1 + f2;
}

if (g >= 10)
{
g1 = g % 10;
g2 = g / 10;
g = g1 + g2;
}

if (h >= 10)
{
h1 = h % 10;
h2 = h / 10;
h = h1 + h2;
}

cksum = a + b + c + d + e + f + g + h + i + j + k + l + m + n + o + p;

if (ccnum >= 340000000000000 && ccnum <= 349999999999999 && cksum % 10 == 0)
{
printf("AMEX\n");
return 0;
}

if (ccnum >= 370000000000000 && ccnum <= 379999999999999 && cksum % 10 == 0)
{
printf("AMEX\n");
return 0;
}

if (ccnum >= 5100000000000000 && ccnum <= 5599999999999999 && cksum % 10 == 0)
{
printf("MASTERCARD\n");
return 0;
}

if (ccnum >= 4000000000000 && ccnum <= 4999999999999 && cksum % 10 == 0)
{
printf("VISA\n");
return 0;
}

if (ccnum >= 4000000000000000 && ccnum <= 4999999999999999 && cksum % 10 == 0)
{
printf("VISA\n");
return 0;
}
else
{
printf("INVALID\n");
return 0;
}

}```

2. Get rid of the "return 0s in the first "do .. while" loop. Then break out the validity check into a separate function and coment it - it's not obvious from the logic what the rationale is, though looking later, it seems to be valid ranges for credit cards rather than anything mathematical.

So the first half of the function becomes

Code:
```do
{
ccnum = get_long("Enter number);
if(!valid_number(ccnum))
printf("INvalid\n");
} while(!valid_number(ccnum));```
;

Now you can replace the long lists of variables with two arrays, because there is a pattern to the logic. One array for the first set, one array for the second set.
Then you just have one test for the value going over 9, in the loop.

Finally you sum by looping instead of explicitly stating all the variables.

3. One other thing: the "long" type is only guaranteed to support the range -2147483647 to +2147483647. That's not big enough to store a credit card number. (A "long" is 64 bits wide on 64-bit *nix systems, which is big enough, but a "long" is only 32 bits wide on 32-bit systems and even on 64-bit Windows, which isn't big enough.)

For "numbers" that aren't mathematical by nature (such as PINs and phone numbers and credit card numbers), it's often better to store them in a string which can be any length. Then you can easily extract any digit from the number.

4. Thanks guys

5. > Is there any way I could have grouped the letter int variable,
Yeah, they're called arrays.

Code:
```int digits[16];

/*
i = ccnum % 10;
j = ccnum % 1000 / 100;
k = ccnum % 100000 / 10000;
l = ccnum % 10000000 / 1000000;
m = ccnum % 1000000000 / 100000000;
n = ccnum % 100000000000 / 10000000000;
o = ccnum % 10000000000000 / 1000000000000;
p = ccnum % 1000000000000000 / 100000000000000;
*/
m = 10; d = 1;
for ( i = 0 ; i < 16 ; i += 2 ) {
digits[i] = ccnum % m / d;
m *= 100;
d *= 100;
}```
Another similar for loop would replace your a - h variables as well.