1

I'm currently working on week 6 python and the problem sets are the same to that of pset 1 just with python instead of C. So when I decided to write it I saw that my code in C was extremely bad so I wanted to optimize it so I can transform it better into python. After reducing it to 120 lines of code when it used to be 255 I get a problem with CHECK50. I tried everything possible and all possible Lhun's algorithm and it still didn't work. The problem was in the last 2 checks of 2 credit cards which are "3400000000000620" and "430000000000000". After multiple tries I decided to put them into a website calculator for Lhun's algorithm and they both were VALID! Just like my program outputted them.

Is there something wrong with my code or is check50 interpreting VALID credit cards as invalid. Just to clarify I tested these 2 cards in many websites and they were VALID and they start with the AMEX and VISA numbers so there is no problem with that: "34" for AMEX and "4" for VISA.

Here is my code

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

int card_length(long number);
bool checksum(long number);

int main(void)
{
    // gets card number from user
    long number = get_long("Number: ");

    // checks for validity of card
    if (checksum(number) == false)
    {
        printf("INVALID\n");
        return 0;
    }

    // gets first and second digits
    while (number > 100)
    {
        number = number / 10;
    }
    int firstDigit = number / 10;
    int firstNumber = number;

    // checks for type of card
    if (firstNumber == 34 || firstNumber == 37)
    {
        printf("AMEX\n");
    }
    else if (firstNumber >= 51 && firstNumber <= 55)
    {
        printf("MASTERCARD\n");
    }
    else if (firstDigit == 4)
    {
        printf("VISA\n");
    }
    else
    {
        printf("INVALID\n");
    }
}


bool checksum(long number)
{
    int length = card_length(number);
    if (length == 1)
    {
        return false;
    }
    // for first half
    long digitLocation = 1;
    int firstSum = 0;
    while (digitLocation < length)
    {
        // gets second to last digit
        long digit = number / pow(10, digitLocation);
        digit %= 10;
        // multiplies digit by 2
        digit *= 2;
        if (digit > 9)
        {
            digit -= 9;
            firstSum += digit;
        }
        else
        {
            firstSum += digit;
        }
        digitLocation += 2;
    }
    // for second half
    digitLocation = 0;
    int secondSum = 0;
    while (digitLocation < length)
    {
        // gets second to last digit
        long digit = number / pow(10, digitLocation);
        digit %= 10;
        secondSum += digit;
        digitLocation += 2;
    }
    int lastSum = firstSum + secondSum;
    if (lastSum % 10 == 0)
    {
        return true;
    }
    else
    {
        return false;
    }
}

int card_length(long number)
{
    int counter = 0;
    double length = number;
    while (length > 0.9999)
    {
        length /= 10;
        counter++;
    }
    if (counter == 13 || counter == 15 || counter == 16)
    {
        return counter;
    }
    else
    {
        return 1;
    }
}

2 Answers 2

0

Similar to @DinoCoderSaurus these are numbers are invalid for a different reason that you have removed a check for in your code.

Per the spec, each credit card type has an associated number of digits the card should have which are valid, and other lengths are not valid. You have removed that checking in the process of your refactor. From the spec:

American Express uses 15-digit numbers, MasterCard uses 16-digit numbers, and Visa uses 13- and 16-digit numbers

Furthermore, while you may have reduced the lines of code ( which is generally good ) you still have a significant optimization that you are missing. I would suggest that you perform the low computation operations to determine that the card number is of appropriate length and key digits such that it even COULD BE a valid credit card number for the various types. Save which type it could be if valid and if its not invalid THEN perform your Luhn's check. If it passes that then print out the previously saved proper type. Otherwise it's invalid. Now you are also reducing the work the CPU is doing which is arguably just as if not more important than reducing the code length.

Also I would suggest searching the web to see if there are more efficient ways to find out how many digits are in a number. As you can do that in a single operation instead of a while loop.... or consider not treating the input as a 'number' but as a string, you can make sure the characters are all valid numbers very easily. And if its a string you dont have to do so much division and can access the letters and the length much more easily.

Also I would suggest not having card_length do anything other than return the length of the card number, and keep the business logic of what card lengths are acceptable elsewhere. Similarly I would probably do a card_length check before even doing a checksum, and before checking which card it could potentially be if the checksum was valid.

Lastly at the end of checksum instead of if (....) { return true; } else { return false; } just do return (....);

0

By my reckoning, neither is valid according to the spec. Program has no correlation between type and length. Neither number is the correct length for the type of card output.

You must log in to answer this question.

Not the answer you're looking for? Browse other questions tagged .