2
\$\begingroup\$
    #include <stdio.h>
    #include <cs50.h>

int main(void){

     long long card;
     int i;
     int otherdigs;
     int multiply;
     int sum = 0;
     int sumofothers = 0;
     int finaladd;
     int length;                      
     int card_length;                 
     long long get_card;

     get_card = get_long_long(" Enter credit card: ");
     card = get_card;




     for (i = 0; i < 16; i++)
     {

       sumofothers += card % 10;

       card /= 10;
       otherdigs = card % 10;
       card /= 10;


       if (otherdigs*2 < 10)
        {
         multiply = otherdigs * 2;
         sum += multiply;
         }

       else
         {
          otherdigs = otherdigs*2-9;
          sum += otherdigs;
         }

       }

      finaladd = sum + sumofothers;

      if (finaladd % 10 == 0)
      {
        for (length = 0; length < 14; length++)
        {
            get_card /= 10;
        }

        if ( get_card >= 51 && get_card < 56 ) {

            printf("MasterCart\n");
        }

        else if ( get_card >= 3 && get_card < 4)
        {
            printf("AmericanExpress\n");
        }

        else if ( get_card < 0 || (get_card/10 >= 4 && get_card/10 < 5) )
        {
            printf("VIZA\n");
        }

        else {

            printf(" Sorry, We Don't Support This Payment Company\n");
        }
      }

      else
      {
        printf("This credit card is not valid \n");
      }


      return 0;


}

I recently took cs50 online course and this is my solution to pset1 ( Credit ) This is my very first program that I actually thought through and did it all myself. The goal is to get the card number from user and check if it's valid ( Luhn’s Algorithm )

I know the style is awful and the code is very messy too, but your opinion is very important to me... I spent maybe 5,6 hours for this. The question is do you think I can code? Become a programmer ? I mean do I have the right mind for it ? ( since this is my first without guide code ) BTW, I have no prior programming experience (only read some pages of K&R 2 years ago),

I even don't know half of c syntax yet. I know talent is a myth and all, but based on this code, do you think I'm any good at this?

\$\endgroup\$
  • \$\begingroup\$ Have you submitted this for a grade yet? \$\endgroup\$ – pacmaninbw May 23 at 3:16
  • \$\begingroup\$ Welcome to Code Review. The former question title, which stated your concerns about the code, applied to too many questions on this site to be useful. The site standard is for the title to simply state the task accomplished by the code. I've changed it to reflect the goal of your code. \$\endgroup\$ – Zeta May 23 at 4:16
5
\$\begingroup\$

the cs50.h that I have does not have a function: get_long_long(), so, for me, the code does not compile. My compiler suggested: getlonglong()

which credit card company is, normally, contained in the left digits of the card number, not in the right digits. so the posted code logic is (probably) not correct.

The posted code fails to take into account the security code found on the back of the credit card.

Not all credit cards use 16 digits, but rather varies by which vendor is producing the card. So that assumption should not be in the code logic.

Suggest reading the digits as a string rather than as a long long int. Remember that a character digit - '0' results in a value 0...9, so the conversion, digit by digit is easy.

OT: when compiling, always enable the warnings, then fix those warnings. ( for gcc, at a minimum use: -Wall -Wextra -Wconversion -pedantic -std=gnu11 ) Note other compilers use different options to produce the same results.

the posted code, when run through the compiler, results in the following:

gcc   -O1  -ggdb -Wall -Wextra -Wconversion -pedantic -std=gnu11  -c "untitled1.c"  -I.

untitled1.c: In function ‘main’:

untitled1.c:17:17: warning: implicit declaration of function ‘get_long_long’; did you mean ‘GetLongLong’? [-Wimplicit-function-declaration]
      get_card = get_long_long(" Enter credit card: ");
                 ^~~~~~~~~~~~~
                 GetLongLong

untitled1.c:26:23: warning: conversion to ‘int’ from ‘long long int’ may alter its value [-Wconversion]
        sumofothers += card % 10;
                       ^~~~

untitled1.c:29:20: warning: conversion to ‘int’ from ‘long long int’ may alter its value [-Wconversion]
        otherdigs = card % 10;
                    ^~~~

untitled1.c:14:10: warning: unused variable ‘card_length’ [-Wunused-variable]
      int card_length;
          ^~~~~~~~~~~

Compilation finished successfully.

regarding: Compilation finished successfully This only means the compiler provided some workaround for each of the problems it found in the code. It does NOT mean the correct/expected code was produced.

Regarding you question about your suitability to become a programmer.

All programmers started some where, usually fumbling through many many programs before they started thinking of problems in terms of the needed code to solve the problem, so your doing quite well and will get better and better as you gain experience.

regarding:

I even don't know half of c syntax yet.

the C language is not that big. besides what you have in the posted code, there are pointers and the while() and switch() statement. Note: learning pointers can be a real pain. After that there are many many standard C library functions. In Linux, in a 'terminal' window, type man functionName to learn all about a function. The 'man' command is your best friend, After some 40+ years of programming, I still use that function regularly.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ @chux-ReinstateMonica, I'll remove that statement \$\endgroup\$ – user3629249 May 23 at 23:00
  • \$\begingroup\$ thank you for your answer, the logic is correct but not perfect and it works as you can see in the second loop, card number will divide until it reaches the 2 first left digits. regarding to security code it wasn't discussed in pset so i didn't include that, and for different digits length I'll use while instead of for loop so it's better optimized. thank you again \$\endgroup\$ – arverse May 25 at 22:28
3
\$\begingroup\$

The question is do you think I can code?
Become a programmer ?
I mean do I have the right mind for it ?
do you think I'm any good at this?

Yes, but code review is more on the code than the programmer.

I know the style is awful and the code is very messy too, but your opinion is very important to me.

Use an auto formatter.

Many coding environments have one. Tip: well formatted code makes a good impression. Do not manual format - life is too short for that.

Enable all warnings

As well suggested by @user3629249, this saves you time and avoids learner mistakes.

Validate range from users

Users are notorious for bad input. Do not trust the input until vetted.

#define CARD_MAX 9999999999999999  /* Or whatever your limit */
if (get_card < 0 || get_card > CARD_MAX) {
  printf("This credit card is not valid \n");
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wouldn't 999999999999999 without any L suffix generate an error? \$\endgroup\$ – Roland Illig May 25 at 7:18
  • \$\begingroup\$ @RolandIllig No. Why do you think it might? \$\endgroup\$ – chux - Reinstate Monica May 25 at 12:17
  • \$\begingroup\$ I thought it would overflow an int, just like in Java. I don't remember that C had unbounded integer literals. I'll read it up later. \$\endgroup\$ – Roland Illig May 25 at 17:19
  • 1
    \$\begingroup\$ Cool, I didn't remember 6.4.4.1p5. Thank you for making me look it up. \$\endgroup\$ – Roland Illig May 25 at 21:19
  • 1
    \$\begingroup\$ @arverse I do have a ready reference for good code, I recommend reading highly rated reviews here and set aside K&R as a reference - it was good in the day. \$\endgroup\$ – chux - Reinstate Monica May 26 at 0:32

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.