Mfcttrf

I am relatively new to C programming and I am currently working through the CS50 EDX course. The problem I have solved below is for week 1 (credit).

Any suggestions on how to improve this code to further my learning would be most appreciated. I feel like my solution is very clunky although it does the job!

#include <cs50.h>
#include <stdio.h>
#include <math.h>

int main(void)

 long number = get_long("Number: ");

 // get the individual intergers of number
 // *2 odd digits

 int i1 = ((number / 1000000000000000) % 10); 
 int t1 = i1 * 2;
 int i2 = ((number / 100000000000000) % 10);
 int i3 = ((number / 10000000000000) % 10);
 int t3 = i3 * 2; 
 int i4 = ((number / 1000000000000) % 10);
 int i5 = ((number / 100000000000) % 10);
 int t5 = i5 * 2;
 int i6 = ((number / 10000000000) % 10);
 int i7 = ((number / 1000000000) % 10);
 int t7 = i7 * 2;
 int i8 = ((number / 100000000) % 10);
 int i9 = ((number / 10000000) % 10);
 int t9 = i9 * 2;
 int i10 = ((number / 1000000) % 10);
 int i11 = ((number / 100000) % 10);
 int t11 = i11 * 2;
 int i12 = ((number / 10000) % 10);
 int i13 = ((number / 1000) % 10);
 int t13 = i13 * 2;
 int i14 = ((number / 100) % 10);
 int i15 = ((number / 10) % 10);
 int t15 = i15 * 2;
 int i16 = (number % 10);

 // Luhns Alg
 // calculate sum of variable digits if > 9

 if (t1>9)
 t1 = t1 - 9;
 if (t3>9)
 t3 = t3 - 9;
 if (t5>9)
 t5 = t5 - 9;
 if (t7>9)
 t7 = t7 - 9;
 if (t9>9)
 t9 = t9 - 9;
 if (t11>9)
 t11 = t11 - 9;
 if (t13>9)
 t13 = t13 - 9;
 if (t15>9)
 t15 = t15 - 9;

 // check lunghs algo = true (0)
 // print card type 

 int sum = (t1+t3+t5+t7+t9+t11+t13+t15+i2+i4+i6+i8+i10+i12+i14+i16); 
 int check = (sum % 10);

 if (check != 0)
 printf("INVALIDn");
 else
 i2 == 5))
 
 printf("MASTERCARDn");
 
 else if (i1 == 4)
 
 printf("VISAn");
 
 else 
 printf("INVALIDn");
 

The code currently outputs the correct results below.

378282246310005 = AMEX

5555555555554444 = MASTERCARD

5105105105105100 = MASTERCARD

4111111111111111 = VISA

5673598276138003 = INVALID

4062901840 = INVALID

It might be better to treat the credit card number as a string. In the C programming language a string is a null terminated array of type char or character. This would remove all the division in the program to get each character. It would also allow the program to detect if any non-numeric characters were entered. The first test to see if it is valid would then be to check if there are 15 or 16 numeric characters in the string, all valid credit card numbers have either 15 or 16 numbers in them (some AMEX cards have 15 numeric characters, all other credit cards have 16 numeric characters).

To get the actual numeric value of a character you would subtract 0 from the numeric character. For a single character this would always give you a value between zero and nine.

Variable Names

Having variables i1 through i16 is a very good indication that i should be an array, this is also true of t.

Having single character variable names is generally frowned upon except for loop control values. A single character really doesn't tell anyone reading or modifying the code what the variable really is or does. It isn't really clear in the program what i or t represents. While number is longer, it might be better if it was credit_card_number.

Basic Principles When Writing Code

One of the earliest principles all programmers learn is Don't Repeat Yourself, usually shortened to DRY code. Basically anytime you have code that repeats it would be better to put it into a loop or a function where the code is reused.

One example of repeating code from the question is :

 if (tN > 9)
 
 tN = tN - 9;
 

This code can be made into a function:

int adjust_t_value(int tN)

 if (tN > 9)
 
 return tN - 9;
 
 else
 
 return tN;
 

If the variable t was an array, then code in the program could be reduced to

 for (int t_count = 0; t_count < N; t_count++)
 
 t[t_count] = adjust_t_value(t[t_count]);
 

There is a second form of the if statement that could also make the code shorter, it is generally covered in the later part of any C programming course

 tN = (tN > 9)? tN - 9 : tN;

This single statement is equivalent to the function above.

A second example of repeating code is the division to reduce each digit in the credit card number to a single number, this could also be put into a loop. The divisor could be reduced in each iteration of the loop if the algorithm sticks with using numbers.

A second principle that should be taught early but is part of more complex programming is the Single Responsibility Principle which states that every module, class, or function should have responsibility over a single part of the functionality provided by the software, and that responsibility should be entirely encapsulated within the function, module or class. This reduces the size of functions which allows functions to be more readable, maintainable and easier to debug. This would mean breaking the main() function into two or three sub functions to reduce the complexity of main.

Use Vertical Spacing to Make the Code More Readable

The code in the question has the if (tN > 9) on only two lines, it might be more readable if it was 4 lines as shown above.