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

  1. #1
    Registered User
    Join Date
    May 2020
    Posts
    3

    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. #2
    Registered User
    Join Date
    May 2012
    Posts
    395
    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.
    I'm the author of MiniBasic: How to write a script interpreter and Basic Algorithms
    Visit my website for lots of associated C programming resources.
    https://github.com/MalcolmMcLean


  3. #3
    Registered User
    Join Date
    May 2012
    Location
    Arizona, USA
    Posts
    725
    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. #4
    Registered User
    Join Date
    May 2020
    Posts
    3
    Thanks guys

  5. #5
    and the hat of int overfl Salem's Avatar
    Join Date
    Aug 2001
    Location
    The edge of the known universe
    Posts
    38,072
    > 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.
    If you dance barefoot on the broken glass of undefined behaviour, you've got to expect the occasional cut.
    If at first you don't succeed, try writing your phone number on the exam paper.

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. how can l make this shorter?
    By am687876 in forum C Programming
    Replies: 3
    Last Post: 05-28-2013, 11:12 AM
  2. Please critique and suggest improvements for parser
    By Queue in forum C Programming
    Replies: 14
    Last Post: 09-28-2006, 08:28 PM
  3. Shorter notation?
    By GaPe in forum C Programming
    Replies: 2
    Last Post: 06-14-2003, 11:35 AM
  4. Any way to make shorter???
    By Unregistered in forum C Programming
    Replies: 3
    Last Post: 02-14-2002, 04:02 AM
  5. is there a better/shorter way to do a menu
    By sizzle_chest in forum C++ Programming
    Replies: 3
    Last Post: 10-22-2001, 04:09 PM

Tags for this Thread