Mixing
Determining whether a provided credit card number is valid according to Luhn’s algorithm
Clash Royale CLAN TAG#URR8PPP
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty margin-bottom:0;
up vote
5
down vote
favorite
I have been working on credit problem for edx CS50 and got it working as intended. an issue I find with it is the way I have to reinitialise the cardNumber variable each time it enters a loop, and there are plenty of loops in my solution. Same goes for my bool value. Is there a better, cleaner way to go around this?
#include <stdio.h>
#include <cs50.h>
// Implement a program that determines whether a provided credit card number
// is valid according to Luhn’s algorithm.
int main(void)
long long userInput;
// Prompt for user input as long long.
do
userInput = get_long_long("Enter credit card number to validate: ");
// Ensure user inputs integer otherwise prompt "Retry: "
if (userInput < 0)
printf("Retry: n");
while (userInput < 0);
long long cardNumber = userInput;
int checkSum = 0;
int secDig, lastDig, multDig;
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
while (cardNumber > 0)
// sum digits that weren't multiplied.
lastDig = (cardNumber % 10);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += lastDig;
secDig = (cardNumber / 10) % 10;
cardNumber /= 100;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
// validate checksum
// if sum of digits ends in 0 ergo % 10 = 0 then number is valid.
bool valid = false;
if (checkSum % 10 == 0)
valid = true;
int numOfDigits = 0;
cardNumber = userInput;
// validate number's length
// track number's length
while (cardNumber > 0)
cardNumber /= 10;
numOfDigits++;
cardNumber = userInput;
// validate company's identifier
// if 15 digits, starts with 34 or 37 - AMEX
if (valid == true && numOfDigits > 12)
else
printf("INVALIDn");
All comments are part of pseudocode for this solution. I left them there to satisfy style50. I would appreciate advice so I can write cleaner next time and make it into a habit. I am a beginner starting CS degree next year with only experience with doing C# courses with Mosh Hamedani.
beginner c checksum
edited 9 hours ago
200_success
123k14143398
asked 12 hours ago
Karim
283
add a comment |Â
up vote
5
down vote
favorite
I have been working on credit problem for edx CS50 and got it working as intended. an issue I find with it is the way I have to reinitialise the cardNumber variable each time it enters a loop, and there are plenty of loops in my solution. Same goes for my bool value. Is there a better, cleaner way to go around this?
#include <stdio.h>
#include <cs50.h>
// Implement a program that determines whether a provided credit card number
// is valid according to Luhn’s algorithm.
int main(void)
long long userInput;
// Prompt for user input as long long.
do
userInput = get_long_long("Enter credit card number to validate: ");
// Ensure user inputs integer otherwise prompt "Retry: "
if (userInput < 0)
printf("Retry: n");
while (userInput < 0);
long long cardNumber = userInput;
int checkSum = 0;
int secDig, lastDig, multDig;
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
while (cardNumber > 0)
// sum digits that weren't multiplied.
lastDig = (cardNumber % 10);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += lastDig;
secDig = (cardNumber / 10) % 10;
cardNumber /= 100;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
// validate checksum
// if sum of digits ends in 0 ergo % 10 = 0 then number is valid.
bool valid = false;
if (checkSum % 10 == 0)
valid = true;
int numOfDigits = 0;
cardNumber = userInput;
// validate number's length
// track number's length
while (cardNumber > 0)
cardNumber /= 10;
numOfDigits++;
cardNumber = userInput;
// validate company's identifier
// if 15 digits, starts with 34 or 37 - AMEX
if (valid == true && numOfDigits > 12)
else
printf("INVALIDn");
All comments are part of pseudocode for this solution. I left them there to satisfy style50. I would appreciate advice so I can write cleaner next time and make it into a habit. I am a beginner starting CS degree next year with only experience with doing C# courses with Mosh Hamedani.
beginner c checksum
edited 9 hours ago
200_success
123k14143398
asked 12 hours ago
Karim
283
add a comment |Â
up vote
5
down vote
favorite
up vote
5
down vote
favorite
I have been working on credit problem for edx CS50 and got it working as intended. an issue I find with it is the way I have to reinitialise the cardNumber variable each time it enters a loop, and there are plenty of loops in my solution. Same goes for my bool value. Is there a better, cleaner way to go around this?
#include <stdio.h>
#include <cs50.h>
// Implement a program that determines whether a provided credit card number
// is valid according to Luhn’s algorithm.
int main(void)
long long userInput;
// Prompt for user input as long long.
do
userInput = get_long_long("Enter credit card number to validate: ");
// Ensure user inputs integer otherwise prompt "Retry: "
if (userInput < 0)
printf("Retry: n");
while (userInput < 0);
long long cardNumber = userInput;
int checkSum = 0;
int secDig, lastDig, multDig;
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
while (cardNumber > 0)
// sum digits that weren't multiplied.
lastDig = (cardNumber % 10);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += lastDig;
secDig = (cardNumber / 10) % 10;
cardNumber /= 100;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
// validate checksum
// if sum of digits ends in 0 ergo % 10 = 0 then number is valid.
bool valid = false;
if (checkSum % 10 == 0)
valid = true;
int numOfDigits = 0;
cardNumber = userInput;
// validate number's length
// track number's length
while (cardNumber > 0)
cardNumber /= 10;
numOfDigits++;
cardNumber = userInput;
// validate company's identifier
// if 15 digits, starts with 34 or 37 - AMEX
if (valid == true && numOfDigits > 12)
else
printf("INVALIDn");
All comments are part of pseudocode for this solution. I left them there to satisfy style50. I would appreciate advice so I can write cleaner next time and make it into a habit. I am a beginner starting CS degree next year with only experience with doing C# courses with Mosh Hamedani.
beginner c checksum
edited 9 hours ago
200_success
123k14143398
asked 12 hours ago
Karim
283
I have been working on credit problem for edx CS50 and got it working as intended. an issue I find with it is the way I have to reinitialise the cardNumber variable each time it enters a loop, and there are plenty of loops in my solution. Same goes for my bool value. Is there a better, cleaner way to go around this?
#include <stdio.h>
#include <cs50.h>
// Implement a program that determines whether a provided credit card number
// is valid according to Luhn’s algorithm.
int main(void)
long long userInput;
// Prompt for user input as long long.
do
userInput = get_long_long("Enter credit card number to validate: ");
// Ensure user inputs integer otherwise prompt "Retry: "
if (userInput < 0)
printf("Retry: n");
while (userInput < 0);
long long cardNumber = userInput;
int checkSum = 0;
int secDig, lastDig, multDig;
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
while (cardNumber > 0)
// sum digits that weren't multiplied.
lastDig = (cardNumber % 10);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += lastDig;
secDig = (cardNumber / 10) % 10;
cardNumber /= 100;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
// validate checksum
// if sum of digits ends in 0 ergo % 10 = 0 then number is valid.
bool valid = false;
if (checkSum % 10 == 0)
valid = true;
int numOfDigits = 0;
cardNumber = userInput;
// validate number's length
// track number's length
while (cardNumber > 0)
cardNumber /= 10;
numOfDigits++;
cardNumber = userInput;
// validate company's identifier
// if 15 digits, starts with 34 or 37 - AMEX
if (valid == true && numOfDigits > 12)
else
printf("INVALIDn");
All comments are part of pseudocode for this solution. I left them there to satisfy style50. I would appreciate advice so I can write cleaner next time and make it into a habit. I am a beginner starting CS degree next year with only experience with doing C# courses with Mosh Hamedani.
beginner c checksum
edited 9 hours ago
200_success
123k14143398
asked 12 hours ago
Karim
283
edited 9 hours ago
200_success
123k14143398
edited 9 hours ago
200_success
123k14143398
edited 9 hours ago
200_success
123k14143398
123k14143398
asked 12 hours ago
Karim
283
asked 12 hours ago
Karim
283
asked 12 hours ago
Karim
283
283
add a comment |Â
add a comment |Â
2 Answers
2
active
oldest
votes
up vote
4
down vote
accepted
First thing I think of is that you have a pretty long main. You should split the code in more functions. Your code could look like this:
long long readCardnumber();
bool validateChecksum(long long cardnumber);
typedef enum invalid, amex, mastercard, visa company;
company getCompany(long long cardnumber);
int main()
long long cardnumber = readCardnumber();
bool valid = validateCardnumber(cardnumber);
if(!valid)
printf("INVALIDn");
exit(0);
company comp = getCompany(long long cardnumber);
switch(comp)
case visa: printf("VISAn"); break;
case mastercard: printf("MASTERCARDn"); break;
case amex: printf("AMEXn"); break;
default: printf("INVALIDn");
Another thing is that you are writing if (valid == false). Just write if (!valid) instead. At least I think it looks way better, since it feels natural to read it as "if not valid". Also, write if (valid) instead of if (valid == true).
One general advice on boolean expressions. Consider writing if(3 == var) instead of if(var == 3). The reason is if you accidentally write = instead of == you will get compiling code with an annoying bug if you write the constant on the right. If you have it on the left, you will instead get a compilation error, which is much nicer to handle. (I don't do this myself though, and the only reason is that I don't like how it looks.)
There are some other stuff that I would consider bad design, like when you are resetting the value of cardNumber several times. However, this is a direct consequence of not having separate functions.
answered 11 hours ago
klutt
1766
I understand looking from C# perspective. I guess main issue might be that I haven't got to defining functions in c yet. Thanks, might be a good exercise to get back to this code when I learn more C syntax.
– Karim
11 hours ago
1
@Karim Using functions to structure your code is probably the most important tool to make code well designed. I definitely don't want to discourage you, because it is great that you care about this. However, until you know how to use functions I would not worry to much about it.
– klutt
11 hours ago
Following your suggestions I change the code to implement functions. paste.ofcode.org/33EZAcBeTpWNbPj6BZyFc9H I believe it looks and feels better. Great advice.
– Karim
8 hours ago
@Karim Nice! Feel free to post a new question here with your updated code. It's much easier to give advice for the rest now.
– klutt
7 hours ago
add a comment |Â
up vote
3
down vote
This is a pretty good start. Your code is readable and understandable. Your variable names are descriptive, and you've used the right types for the work you're doing. Here are a few suggestions for improvements.
Dealing With Strings vs. Numbers
This is a case where some of your code would be easier to write if you were dealing with strings instead of numbers. You're fortunate in that a 13-16 digit number fits inside of a long long, and there should be no valid credit card numbers with leading 0s. That gets rid of 2 problematic issues with validating numerical inputs.
If you obtained the user's input as a string instead of a long long, you'd need to convert the characters to values so you could check the checksum. At first that seems like more work. But here's a function that converts a char to an int:
int charToInt(const char c)
if (isdigit(c))
return (int)(c - `0`);
return -1;
Once you have that, getting the digits out of the card is about as easy as doing it numerically, but you don't have to keep dividing the number and making copies of it. Your checksum loop ends up looking like this:
char* cardNumber; // Assume this was gotten above and is filled out correctly
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
int nextDigit;
unsigned long numberLen = strlen(cardNumber);
char* nextDigitChar = cardNumber + numberLen - 1;
while (nextDigitChar > cardNumber)
// sum digits that weren't multiplied.
nextDigit = charToInt(*nextDigitChar);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += nextDigit;
nextDigitChar--;
// Get the security digit
secDig = charToInt(*nextDigitChar);
nextDigitChar--;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
Now this probably doesn't look like a whole lot less work than what you were doing before. However, notice that we haven't modified the input at all. So we don't need to copy it again to do other work. Speaking of which...
I would check the length of the input before doing any other work. If it isn't 13-16 characters, then there's no point in doing any checksum because we already know it's invalid. Note also, that this makes the last part of your program much easier, too. You can get the first 2 digits by simply looking at the string.
if (valid)
if (!valid)
printf("INVALIDn");
answered 10 hours ago
user1118321
10k11144
add a comment |Â
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
up vote
4
down vote
accepted
First thing I think of is that you have a pretty long main. You should split the code in more functions. Your code could look like this:
long long readCardnumber();
bool validateChecksum(long long cardnumber);
typedef enum invalid, amex, mastercard, visa company;
company getCompany(long long cardnumber);
int main()
long long cardnumber = readCardnumber();
bool valid = validateCardnumber(cardnumber);
if(!valid)
printf("INVALIDn");
exit(0);
company comp = getCompany(long long cardnumber);
switch(comp)
case visa: printf("VISAn"); break;
case mastercard: printf("MASTERCARDn"); break;
case amex: printf("AMEXn"); break;
default: printf("INVALIDn");
Another thing is that you are writing if (valid == false). Just write if (!valid) instead. At least I think it looks way better, since it feels natural to read it as "if not valid". Also, write if (valid) instead of if (valid == true).
One general advice on boolean expressions. Consider writing if(3 == var) instead of if(var == 3). The reason is if you accidentally write = instead of == you will get compiling code with an annoying bug if you write the constant on the right. If you have it on the left, you will instead get a compilation error, which is much nicer to handle. (I don't do this myself though, and the only reason is that I don't like how it looks.)
There are some other stuff that I would consider bad design, like when you are resetting the value of cardNumber several times. However, this is a direct consequence of not having separate functions.
answered 11 hours ago
klutt
1766
I understand looking from C# perspective. I guess main issue might be that I haven't got to defining functions in c yet. Thanks, might be a good exercise to get back to this code when I learn more C syntax.
– Karim
11 hours ago
1
@Karim Using functions to structure your code is probably the most important tool to make code well designed. I definitely don't want to discourage you, because it is great that you care about this. However, until you know how to use functions I would not worry to much about it.
– klutt
11 hours ago
Following your suggestions I change the code to implement functions. paste.ofcode.org/33EZAcBeTpWNbPj6BZyFc9H I believe it looks and feels better. Great advice.
– Karim
8 hours ago
@Karim Nice! Feel free to post a new question here with your updated code. It's much easier to give advice for the rest now.
– klutt
7 hours ago
add a comment |Â
up vote
4
down vote
accepted
First thing I think of is that you have a pretty long main. You should split the code in more functions. Your code could look like this:
long long readCardnumber();
bool validateChecksum(long long cardnumber);
typedef enum invalid, amex, mastercard, visa company;
company getCompany(long long cardnumber);
int main()
long long cardnumber = readCardnumber();
bool valid = validateCardnumber(cardnumber);
if(!valid)
printf("INVALIDn");
exit(0);
company comp = getCompany(long long cardnumber);
switch(comp)
case visa: printf("VISAn"); break;
case mastercard: printf("MASTERCARDn"); break;
case amex: printf("AMEXn"); break;
default: printf("INVALIDn");
Another thing is that you are writing if (valid == false). Just write if (!valid) instead. At least I think it looks way better, since it feels natural to read it as "if not valid". Also, write if (valid) instead of if (valid == true).
One general advice on boolean expressions. Consider writing if(3 == var) instead of if(var == 3). The reason is if you accidentally write = instead of == you will get compiling code with an annoying bug if you write the constant on the right. If you have it on the left, you will instead get a compilation error, which is much nicer to handle. (I don't do this myself though, and the only reason is that I don't like how it looks.)
There are some other stuff that I would consider bad design, like when you are resetting the value of cardNumber several times. However, this is a direct consequence of not having separate functions.
answered 11 hours ago
klutt
1766
I understand looking from C# perspective. I guess main issue might be that I haven't got to defining functions in c yet. Thanks, might be a good exercise to get back to this code when I learn more C syntax.
– Karim
11 hours ago
1
@Karim Using functions to structure your code is probably the most important tool to make code well designed. I definitely don't want to discourage you, because it is great that you care about this. However, until you know how to use functions I would not worry to much about it.
– klutt
11 hours ago
Following your suggestions I change the code to implement functions. paste.ofcode.org/33EZAcBeTpWNbPj6BZyFc9H I believe it looks and feels better. Great advice.
– Karim
8 hours ago
@Karim Nice! Feel free to post a new question here with your updated code. It's much easier to give advice for the rest now.
– klutt
7 hours ago
add a comment |Â
up vote
4
down vote
accepted
up vote
4
down vote
accepted
First thing I think of is that you have a pretty long main. You should split the code in more functions. Your code could look like this:
long long readCardnumber();
bool validateChecksum(long long cardnumber);
typedef enum invalid, amex, mastercard, visa company;
company getCompany(long long cardnumber);
int main()
long long cardnumber = readCardnumber();
bool valid = validateCardnumber(cardnumber);
if(!valid)
printf("INVALIDn");
exit(0);
company comp = getCompany(long long cardnumber);
switch(comp)
case visa: printf("VISAn"); break;
case mastercard: printf("MASTERCARDn"); break;
case amex: printf("AMEXn"); break;
default: printf("INVALIDn");
Another thing is that you are writing if (valid == false). Just write if (!valid) instead. At least I think it looks way better, since it feels natural to read it as "if not valid". Also, write if (valid) instead of if (valid == true).
One general advice on boolean expressions. Consider writing if(3 == var) instead of if(var == 3). The reason is if you accidentally write = instead of == you will get compiling code with an annoying bug if you write the constant on the right. If you have it on the left, you will instead get a compilation error, which is much nicer to handle. (I don't do this myself though, and the only reason is that I don't like how it looks.)
There are some other stuff that I would consider bad design, like when you are resetting the value of cardNumber several times. However, this is a direct consequence of not having separate functions.
answered 11 hours ago
klutt
1766
First thing I think of is that you have a pretty long main. You should split the code in more functions. Your code could look like this:
long long readCardnumber();
bool validateChecksum(long long cardnumber);
typedef enum invalid, amex, mastercard, visa company;
company getCompany(long long cardnumber);
int main()
long long cardnumber = readCardnumber();
bool valid = validateCardnumber(cardnumber);
if(!valid)
printf("INVALIDn");
exit(0);
company comp = getCompany(long long cardnumber);
switch(comp)
case visa: printf("VISAn"); break;
case mastercard: printf("MASTERCARDn"); break;
case amex: printf("AMEXn"); break;
default: printf("INVALIDn");
Another thing is that you are writing if (valid == false). Just write if (!valid) instead. At least I think it looks way better, since it feels natural to read it as "if not valid". Also, write if (valid) instead of if (valid == true).
One general advice on boolean expressions. Consider writing if(3 == var) instead of if(var == 3). The reason is if you accidentally write = instead of == you will get compiling code with an annoying bug if you write the constant on the right. If you have it on the left, you will instead get a compilation error, which is much nicer to handle. (I don't do this myself though, and the only reason is that I don't like how it looks.)
There are some other stuff that I would consider bad design, like when you are resetting the value of cardNumber several times. However, this is a direct consequence of not having separate functions.
answered 11 hours ago
klutt
1766
edited 11 hours ago
answered 11 hours ago
klutt
1766
answered 11 hours ago
klutt
1766
answered 11 hours ago
klutt
1766
1766
I understand looking from C# perspective. I guess main issue might be that I haven't got to defining functions in c yet. Thanks, might be a good exercise to get back to this code when I learn more C syntax.
– Karim
11 hours ago
1
@Karim Using functions to structure your code is probably the most important tool to make code well designed. I definitely don't want to discourage you, because it is great that you care about this. However, until you know how to use functions I would not worry to much about it.
– klutt
11 hours ago
Following your suggestions I change the code to implement functions. paste.ofcode.org/33EZAcBeTpWNbPj6BZyFc9H I believe it looks and feels better. Great advice.
– Karim
8 hours ago
@Karim Nice! Feel free to post a new question here with your updated code. It's much easier to give advice for the rest now.
– klutt
7 hours ago
add a comment |Â
I understand looking from C# perspective. I guess main issue might be that I haven't got to defining functions in c yet. Thanks, might be a good exercise to get back to this code when I learn more C syntax.
– Karim
11 hours ago
1
@Karim Using functions to structure your code is probably the most important tool to make code well designed. I definitely don't want to discourage you, because it is great that you care about this. However, until you know how to use functions I would not worry to much about it.
– klutt
11 hours ago
Following your suggestions I change the code to implement functions. paste.ofcode.org/33EZAcBeTpWNbPj6BZyFc9H I believe it looks and feels better. Great advice.
– Karim
8 hours ago
@Karim Nice! Feel free to post a new question here with your updated code. It's much easier to give advice for the rest now.
– klutt
7 hours ago
I understand looking from C# perspective. I guess main issue might be that I haven't got to defining functions in c yet. Thanks, might be a good exercise to get back to this code when I learn more C syntax.
– Karim
11 hours ago
I understand looking from C# perspective. I guess main issue might be that I haven't got to defining functions in c yet. Thanks, might be a good exercise to get back to this code when I learn more C syntax.
– Karim
11 hours ago
1
1
@Karim Using functions to structure your code is probably the most important tool to make code well designed. I definitely don't want to discourage you, because it is great that you care about this. However, until you know how to use functions I would not worry to much about it.
– klutt
11 hours ago
@Karim Using functions to structure your code is probably the most important tool to make code well designed. I definitely don't want to discourage you, because it is great that you care about this. However, until you know how to use functions I would not worry to much about it.
– klutt
11 hours ago
Following your suggestions I change the code to implement functions. paste.ofcode.org/33EZAcBeTpWNbPj6BZyFc9H I believe it looks and feels better. Great advice.
– Karim
8 hours ago
Following your suggestions I change the code to implement functions. paste.ofcode.org/33EZAcBeTpWNbPj6BZyFc9H I believe it looks and feels better. Great advice.
– Karim
8 hours ago
@Karim Nice! Feel free to post a new question here with your updated code. It's much easier to give advice for the rest now.
– klutt
7 hours ago
@Karim Nice! Feel free to post a new question here with your updated code. It's much easier to give advice for the rest now.
– klutt
7 hours ago
add a comment |Â
up vote
3
down vote
This is a pretty good start. Your code is readable and understandable. Your variable names are descriptive, and you've used the right types for the work you're doing. Here are a few suggestions for improvements.
Dealing With Strings vs. Numbers
This is a case where some of your code would be easier to write if you were dealing with strings instead of numbers. You're fortunate in that a 13-16 digit number fits inside of a long long, and there should be no valid credit card numbers with leading 0s. That gets rid of 2 problematic issues with validating numerical inputs.
If you obtained the user's input as a string instead of a long long, you'd need to convert the characters to values so you could check the checksum. At first that seems like more work. But here's a function that converts a char to an int:
int charToInt(const char c)
if (isdigit(c))
return (int)(c - `0`);
return -1;
Once you have that, getting the digits out of the card is about as easy as doing it numerically, but you don't have to keep dividing the number and making copies of it. Your checksum loop ends up looking like this:
char* cardNumber; // Assume this was gotten above and is filled out correctly
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
int nextDigit;
unsigned long numberLen = strlen(cardNumber);
char* nextDigitChar = cardNumber + numberLen - 1;
while (nextDigitChar > cardNumber)
// sum digits that weren't multiplied.
nextDigit = charToInt(*nextDigitChar);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += nextDigit;
nextDigitChar--;
// Get the security digit
secDig = charToInt(*nextDigitChar);
nextDigitChar--;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
Now this probably doesn't look like a whole lot less work than what you were doing before. However, notice that we haven't modified the input at all. So we don't need to copy it again to do other work. Speaking of which...
I would check the length of the input before doing any other work. If it isn't 13-16 characters, then there's no point in doing any checksum because we already know it's invalid. Note also, that this makes the last part of your program much easier, too. You can get the first 2 digits by simply looking at the string.
if (valid)
if (!valid)
printf("INVALIDn");
answered 10 hours ago
user1118321
10k11144
add a comment |Â
up vote
3
down vote
This is a pretty good start. Your code is readable and understandable. Your variable names are descriptive, and you've used the right types for the work you're doing. Here are a few suggestions for improvements.
Dealing With Strings vs. Numbers
This is a case where some of your code would be easier to write if you were dealing with strings instead of numbers. You're fortunate in that a 13-16 digit number fits inside of a long long, and there should be no valid credit card numbers with leading 0s. That gets rid of 2 problematic issues with validating numerical inputs.
If you obtained the user's input as a string instead of a long long, you'd need to convert the characters to values so you could check the checksum. At first that seems like more work. But here's a function that converts a char to an int:
int charToInt(const char c)
if (isdigit(c))
return (int)(c - `0`);
return -1;
Once you have that, getting the digits out of the card is about as easy as doing it numerically, but you don't have to keep dividing the number and making copies of it. Your checksum loop ends up looking like this:
char* cardNumber; // Assume this was gotten above and is filled out correctly
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
int nextDigit;
unsigned long numberLen = strlen(cardNumber);
char* nextDigitChar = cardNumber + numberLen - 1;
while (nextDigitChar > cardNumber)
// sum digits that weren't multiplied.
nextDigit = charToInt(*nextDigitChar);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += nextDigit;
nextDigitChar--;
// Get the security digit
secDig = charToInt(*nextDigitChar);
nextDigitChar--;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
Now this probably doesn't look like a whole lot less work than what you were doing before. However, notice that we haven't modified the input at all. So we don't need to copy it again to do other work. Speaking of which...
I would check the length of the input before doing any other work. If it isn't 13-16 characters, then there's no point in doing any checksum because we already know it's invalid. Note also, that this makes the last part of your program much easier, too. You can get the first 2 digits by simply looking at the string.
if (valid)
if (!valid)
printf("INVALIDn");
answered 10 hours ago
user1118321
10k11144
add a comment |Â
up vote
3
down vote
up vote
3
down vote
This is a pretty good start. Your code is readable and understandable. Your variable names are descriptive, and you've used the right types for the work you're doing. Here are a few suggestions for improvements.
Dealing With Strings vs. Numbers
This is a case where some of your code would be easier to write if you were dealing with strings instead of numbers. You're fortunate in that a 13-16 digit number fits inside of a long long, and there should be no valid credit card numbers with leading 0s. That gets rid of 2 problematic issues with validating numerical inputs.
If you obtained the user's input as a string instead of a long long, you'd need to convert the characters to values so you could check the checksum. At first that seems like more work. But here's a function that converts a char to an int:
int charToInt(const char c)
if (isdigit(c))
return (int)(c - `0`);
return -1;
Once you have that, getting the digits out of the card is about as easy as doing it numerically, but you don't have to keep dividing the number and making copies of it. Your checksum loop ends up looking like this:
char* cardNumber; // Assume this was gotten above and is filled out correctly
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
int nextDigit;
unsigned long numberLen = strlen(cardNumber);
char* nextDigitChar = cardNumber + numberLen - 1;
while (nextDigitChar > cardNumber)
// sum digits that weren't multiplied.
nextDigit = charToInt(*nextDigitChar);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += nextDigit;
nextDigitChar--;
// Get the security digit
secDig = charToInt(*nextDigitChar);
nextDigitChar--;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
Now this probably doesn't look like a whole lot less work than what you were doing before. However, notice that we haven't modified the input at all. So we don't need to copy it again to do other work. Speaking of which...
I would check the length of the input before doing any other work. If it isn't 13-16 characters, then there's no point in doing any checksum because we already know it's invalid. Note also, that this makes the last part of your program much easier, too. You can get the first 2 digits by simply looking at the string.
if (valid)
if (!valid)
printf("INVALIDn");
answered 10 hours ago
user1118321
10k11144
This is a pretty good start. Your code is readable and understandable. Your variable names are descriptive, and you've used the right types for the work you're doing. Here are a few suggestions for improvements.
Dealing With Strings vs. Numbers
This is a case where some of your code would be easier to write if you were dealing with strings instead of numbers. You're fortunate in that a 13-16 digit number fits inside of a long long, and there should be no valid credit card numbers with leading 0s. That gets rid of 2 problematic issues with validating numerical inputs.
If you obtained the user's input as a string instead of a long long, you'd need to convert the characters to values so you could check the checksum. At first that seems like more work. But here's a function that converts a char to an int:
int charToInt(const char c)
if (isdigit(c))
return (int)(c - `0`);
return -1;
Once you have that, getting the digits out of the card is about as easy as doing it numerically, but you don't have to keep dividing the number and making copies of it. Your checksum loop ends up looking like this:
char* cardNumber; // Assume this was gotten above and is filled out correctly
// start from second to last digit (cc_number / 10) % 10 and multiply every other digit by 2.
int nextDigit;
unsigned long numberLen = strlen(cardNumber);
char* nextDigitChar = cardNumber + numberLen - 1;
while (nextDigitChar > cardNumber)
// sum digits that weren't multiplied.
nextDigit = charToInt(*nextDigitChar);
// add the sum to the sum of the digits that weren't multipied by 2.
checkSum += nextDigit;
nextDigitChar--;
// Get the security digit
secDig = charToInt(*nextDigitChar);
nextDigitChar--;
// multiply every other digit
multDig = secDig * 2;
// add those products (separate /10, return modulus of 10) digits together.
if (multDig > 10)
checkSum += (multDig % 10) + 1;
else if (multDig == 10)
checkSum += 1;
else
checkSum += multDig;
Now this probably doesn't look like a whole lot less work than what you were doing before. However, notice that we haven't modified the input at all. So we don't need to copy it again to do other work. Speaking of which...
I would check the length of the input before doing any other work. If it isn't 13-16 characters, then there's no point in doing any checksum because we already know it's invalid. Note also, that this makes the last part of your program much easier, too. You can get the first 2 digits by simply looking at the string.
if (valid)
if (!valid)
printf("INVALIDn");
answered 10 hours ago
user1118321
10k11144
answered 10 hours ago
user1118321
10k11144
answered 10 hours ago
user1118321
10k11144
answered 10 hours ago
user1118321
10k11144
10k11144
add a comment |Â
add a comment |Â
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f201023%2fdetermining-whether-a-provided-credit-card-number-is-valid-according-to-luhn-s-a%23new-answer', 'question_page');
);
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Post as a guest
Sign up or log in
StackExchange.ready(function ()
StackExchange.helpers.onClickDraftSave('#login-link');
);
Sign up using Google
Sign up using Facebook
Sign up using Email and Password
Sign up using Google
Sign up using Facebook
Sign up using Email and Password