3
\$\begingroup\$

I am beginner a 1st year cs student. I am taking CS50 and I completed the first week problem of implementing Luhn Check algorithm. Though my solution works I feel it's very sloppy and not efficient. Can someone please suggest how can I improve my solution? (It's a first-week problem so you are not intended to use array or functions.)

Here is the code:

#include<stdlib.h>
#include<cs50.h>
int main()
{
    long long x,j=0;
    int len,i,len_flag,flag=0,flag_val=0,sp_len=0,check_flag=0; //basic integers related  to loop etc
    int res,sum,fin_sum=0,h=0,d2_sum=0,d1_sum=0,ff_num=0; // variables  related to extracting numbers 
    int sum_len=0,in_sum=0,in_fsum=0,len_sum=0,m=0; // extraing numbers from more than single  sum result 
    int sum_f=0,sum_final=0;


    do{
        x = get_long("enter a valid card number \n");
        j=x;
        len_flag = 0;
        len = 0;
        while(len_flag!=1)
        {
          x = x / 10;
          if(x==0)
          {
            len_flag = 1;   //finding the lenght of the number 
          }
          len++;
        }
        if(len==15||len==16||len==13)
        {
          flag = 1;
          sp_len =1;
        }
        else
        {
            flag=1;
        }

    }while(flag!=1);


    for(i=0;i<len;i++)
    {
       res = j % 10;
       j = j / 10;
       if(i%2!=0)
       {

        sum = res * 2;
        //printf("multi_res : %d \n",sum);
        len_flag = 0;
        sum_len = 0;
        len_sum = sum;
        while(len_flag!=1)
        {
            len_sum = len_sum / 10;
            if(len_sum==0)
            {
               len_flag=1;       // checking if the sum is more than single  digit 
            }   
            sum_len++;
            //printf("trig\n");
        }
        if(sum_len>1)
        {   x=0;
            while(x<sum_len)
            {
             in_sum = sum % 10;
             sum = sum/10; 
             in_fsum = in_fsum + in_sum; 
             //printf("double sum : %d \n",in_fsum);
             x++;

            }
        }

        fin_sum = fin_sum + sum;

       }
       if(i==len-1)
       {

         for(h=0;h < 1;h++)
         {
            fin_sum = fin_sum + in_fsum; 
         }

         d1_sum = res;
       }

       else if(i%2==0)
       {
        sum_f = sum_f + res; // adding up the number that where not  x2
       }

       if(i==len-2)
       {
         d2_sum = res;     //5555555555554444
       }

    }

    sum_final = sum_f + fin_sum;

    //printf("sum_final : %d\n",sum_final);
    //printf("sum_f_final : %d\n",sum_f);
    //printf("sum_in_final : %d\n",fin_sum);

    if(sum_final%10==0)
    {
        flag_val=1; // checking if the number is valid  
    }
    if(ff_num == 0)
    {                                   
        ff_num = (d1_sum*10) + d2_sum;     //flip
    }

    do
    {
       if(sp_len==0)
       {
        printf("INVALID\n");
        check_flag=1;
        break;
       }
       if((flag==1&&flag_val==1&&ff_num==34)||ff_num==37)
       {
        printf("AMEX\n");
        check_flag=1;
        break;
       }
       else if(flag==1&&flag_val==1&&ff_num>50&&ff_num<56)
       {
        printf("MASTERCARD\n");
        check_flag=1;
        break;
       }
       else if(flag==1&&flag_val==1&&d1_sum==4)
       {
        printf("VISA\n");
        check_flag=1;
        break;
       }
       else
       {
        printf("INVALID\n");
        check_flag=1;
        break;
       }
    }while(check_flag!=1);

    return 0;

}

I felt like I was fixing a leak while writing the code. I would try to correct one thing and another thing would go wrong and this is the final result.

Thank you for your help. If the question was not asked properly please forgive me; it's my first time here.

\$\endgroup\$
  • 1
    \$\begingroup\$ What kind of optimization are you looking for, execution time, memory size or something else? \$\endgroup\$ – pacmaninbw Apr 24 at 17:44
  • \$\begingroup\$ Both i feel there is a better way of doing it but i cant figure it out \$\endgroup\$ – ray man Apr 25 at 4:49
  • \$\begingroup\$ Hmmm, "not intended to use array" --> yet code uses "enter a valid card number \n" which is an array. \$\endgroup\$ – chux - Reinstate Monica Apr 25 at 9:03
3
\$\begingroup\$

Code will look much better once OP can use own functions.

how can I improve my solution?

Meaningful names

Why is the credit card number called j. How about cc_num?

Many others names need like-wise improvements.

Use bool

For flags, consider bool.

#include <stdbool.h>

bool flag;

Auto format

Use a development environment that supports auto formatting. Do not format manually - just not efficient.

#include<stdlib.h>
#include<cs50.h>

int main() {
  long long x, j = 0;
  int len, i, len_flag, flag = 0, flag_val = 0, sp_len = 0, check_flag = 0; //basic integers related  to loop etc
  int res, sum, fin_sum = 0, h = 0, d2_sum = 0, d1_sum = 0, ff_num = 0; // variables  related to extracting numbers 
  int sum_len = 0, in_sum = 0, in_fsum = 0, len_sum = 0, m = 0; // extraing numbers from more than single  sum result 
  int sum_f = 0, sum_final = 0;

  do {
    x = get_long("enter a valid card number \n");
    j = x;
    len_flag = 0;
    len = 0;
    while (len_flag != 1) {
      x = x / 10;
      if (x == 0) {
        len_flag = 1;   //finding the lenght of the number 
      }
      len++;
    }
    if (len == 15 || len == 16 || len == 13) {
      flag = 1;
      sp_len = 1;
    } else {
      flag = 1;
    }

  } while (flag != 1);
  ...

Define variables where needed

Rather than a sea of objects up front, defined them where needed.

Example: Refactor x. Other variables can like-wise get defined closer to their usage.

  // long long x, j = 0;
  long long j = 0;
  ...

  do {
    // x = get_long("enter a valid card number \n");
    long long x = get_long("enter a valid card number \n");
    j = x;
    ...

    if (sum_len > 1) {
      // x = 0;
      int x = 0;

Spelling

//finding the lenght of the number
  finding the length of the number

Clean-up

For review purposes, no need for dead debug code.
Eliminate //printf("multi_res : %d \n",sum), etc.

Eliminate unused variables like m.

Comment code

Each chunk of code deserves to be a function, yet I understand that is prohibited for now.

I'd comment the chunks of code to help aide understanding.

  // Read input CC number
  // Set j as the CC#, len_flag as the CC# length, flag to 1, ...
  do {
    long long x = get_long("enter a valid card number \n");

Simplify

flag only used as 1.

  do {
    ...
      flag = 1;
  } while (flag != 1);

Replaceable with

  {
    ...
  }

Same for later do { ... } while (check_flag != 1);.

Documentation

Within code I'd add an algorithm link:

// https://en.wikipedia.org/wiki/Luhn_algorithm

Why space before '\n' ?

// "enter a valid card number \n"
"enter a valid card number\n"

Chunks of code seem verbose

len_flag = 0;
len = 0;
while (len_flag != 1) {
  x = x / 10;
  if (x == 0) {
    len_flag = 1;   //finding the lenght of the number 
  }
  len++;
}

versus

// decimal digit length of x
len = 0;
do {
  len++;
} while (x /= 10);
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thank you very much will definitely take care of the thing that you pointed out do you have any better way of implementing the algorithm thank you very much \$\endgroup\$ – ray man Apr 25 at 11:07
  • 1
    \$\begingroup\$ @rayman I'd consider processing the CC# as a string and not a number. \$\endgroup\$ – chux - Reinstate Monica Apr 25 at 15:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.