Credit card validator using Luhn Algorithm
.everyoneloves__top-leaderboard:empty,.everyoneloves__mid-leaderboard:empty,.everyoneloves__bot-mid-leaderboard:empty{ margin-bottom:0;
}
$begingroup$
I'm looking for feedback or tips to maybe make my code more readable or faster or just general tips to help get myself started with app making with Python.
import sys
# check for valid input
if len(sys.argv) == 2:
try:
int(sys.argv[1])
if(len(str(sys.argv[1])) == 16):
pass
else:
print("Not 16 digits!")
sys.exit()
except ValueError:
print('Not an integer!')
sys.exit()
else:
print('Not enough or too many command line arguments! n Proper use "python Check.py <credit card number here> " ')
sys.exit()
def main():
# put the digits into a list
number = convertToList(sys.argv[1])
sum = cardCheck(number)
if (sum%10 == 0):
print('Valid Card!')
else:
print('Invalid Card!')
#converts initial passed int variable to list
def convertToList(num):
numStr = str(num)
numList =
for digit in numStr:
numList.append(int(digit))
return (numList)
def cardCheck(digitList, count = 0):
sum = 0
#if digit is every second digit multiply by 2
if(count%2 == 0 & count < 15):
digitList[count] = (digitList[count] * 2)
#if is 2 digit number after multiplication
if(digitList[count] >= 10):
digitList[count] = addDigits(digitList[count])
cardCheck(digitList, count + 1)
else:
cardCheck(digitList, count + 1)
#progresses program
elif(count < 15):
cardCheck(digitList, count + 1)
else:
return 0
for digits in digitList:
sum += int(digits)
return sum
#resolve 2 digit number conflict by adding the digits of the number and returning it
def addDigits(num):
list = str(num)
sum = 0
for digits in list:
sum += int(digits)
return sum
if __name__ == '__main__':
main()
python beginner checksum
$endgroup$
add a comment |
$begingroup$
I'm looking for feedback or tips to maybe make my code more readable or faster or just general tips to help get myself started with app making with Python.
import sys
# check for valid input
if len(sys.argv) == 2:
try:
int(sys.argv[1])
if(len(str(sys.argv[1])) == 16):
pass
else:
print("Not 16 digits!")
sys.exit()
except ValueError:
print('Not an integer!')
sys.exit()
else:
print('Not enough or too many command line arguments! n Proper use "python Check.py <credit card number here> " ')
sys.exit()
def main():
# put the digits into a list
number = convertToList(sys.argv[1])
sum = cardCheck(number)
if (sum%10 == 0):
print('Valid Card!')
else:
print('Invalid Card!')
#converts initial passed int variable to list
def convertToList(num):
numStr = str(num)
numList =
for digit in numStr:
numList.append(int(digit))
return (numList)
def cardCheck(digitList, count = 0):
sum = 0
#if digit is every second digit multiply by 2
if(count%2 == 0 & count < 15):
digitList[count] = (digitList[count] * 2)
#if is 2 digit number after multiplication
if(digitList[count] >= 10):
digitList[count] = addDigits(digitList[count])
cardCheck(digitList, count + 1)
else:
cardCheck(digitList, count + 1)
#progresses program
elif(count < 15):
cardCheck(digitList, count + 1)
else:
return 0
for digits in digitList:
sum += int(digits)
return sum
#resolve 2 digit number conflict by adding the digits of the number and returning it
def addDigits(num):
list = str(num)
sum = 0
for digits in list:
sum += int(digits)
return sum
if __name__ == '__main__':
main()
python beginner checksum
$endgroup$
2
$begingroup$
For the record, if you're just getting started making apps, credit card handling is not the best place, given the immense legal/contractual requirements/standards around them. Most businesses will want to outsource card handling.
$endgroup$
– jpmc26
May 26 at 15:52
$begingroup$
I understand that, I was simply testing my abilities to write a simple checksum algorithm. I plan on writing a program to solve Rubix cubes soon as my first big project.
$endgroup$
– tph
May 26 at 16:23
add a comment |
$begingroup$
I'm looking for feedback or tips to maybe make my code more readable or faster or just general tips to help get myself started with app making with Python.
import sys
# check for valid input
if len(sys.argv) == 2:
try:
int(sys.argv[1])
if(len(str(sys.argv[1])) == 16):
pass
else:
print("Not 16 digits!")
sys.exit()
except ValueError:
print('Not an integer!')
sys.exit()
else:
print('Not enough or too many command line arguments! n Proper use "python Check.py <credit card number here> " ')
sys.exit()
def main():
# put the digits into a list
number = convertToList(sys.argv[1])
sum = cardCheck(number)
if (sum%10 == 0):
print('Valid Card!')
else:
print('Invalid Card!')
#converts initial passed int variable to list
def convertToList(num):
numStr = str(num)
numList =
for digit in numStr:
numList.append(int(digit))
return (numList)
def cardCheck(digitList, count = 0):
sum = 0
#if digit is every second digit multiply by 2
if(count%2 == 0 & count < 15):
digitList[count] = (digitList[count] * 2)
#if is 2 digit number after multiplication
if(digitList[count] >= 10):
digitList[count] = addDigits(digitList[count])
cardCheck(digitList, count + 1)
else:
cardCheck(digitList, count + 1)
#progresses program
elif(count < 15):
cardCheck(digitList, count + 1)
else:
return 0
for digits in digitList:
sum += int(digits)
return sum
#resolve 2 digit number conflict by adding the digits of the number and returning it
def addDigits(num):
list = str(num)
sum = 0
for digits in list:
sum += int(digits)
return sum
if __name__ == '__main__':
main()
python beginner checksum
$endgroup$
I'm looking for feedback or tips to maybe make my code more readable or faster or just general tips to help get myself started with app making with Python.
import sys
# check for valid input
if len(sys.argv) == 2:
try:
int(sys.argv[1])
if(len(str(sys.argv[1])) == 16):
pass
else:
print("Not 16 digits!")
sys.exit()
except ValueError:
print('Not an integer!')
sys.exit()
else:
print('Not enough or too many command line arguments! n Proper use "python Check.py <credit card number here> " ')
sys.exit()
def main():
# put the digits into a list
number = convertToList(sys.argv[1])
sum = cardCheck(number)
if (sum%10 == 0):
print('Valid Card!')
else:
print('Invalid Card!')
#converts initial passed int variable to list
def convertToList(num):
numStr = str(num)
numList =
for digit in numStr:
numList.append(int(digit))
return (numList)
def cardCheck(digitList, count = 0):
sum = 0
#if digit is every second digit multiply by 2
if(count%2 == 0 & count < 15):
digitList[count] = (digitList[count] * 2)
#if is 2 digit number after multiplication
if(digitList[count] >= 10):
digitList[count] = addDigits(digitList[count])
cardCheck(digitList, count + 1)
else:
cardCheck(digitList, count + 1)
#progresses program
elif(count < 15):
cardCheck(digitList, count + 1)
else:
return 0
for digits in digitList:
sum += int(digits)
return sum
#resolve 2 digit number conflict by adding the digits of the number and returning it
def addDigits(num):
list = str(num)
sum = 0
for digits in list:
sum += int(digits)
return sum
if __name__ == '__main__':
main()
python beginner checksum
python beginner checksum
edited May 26 at 3:23
200_success
135k21 gold badges173 silver badges443 bronze badges
135k21 gold badges173 silver badges443 bronze badges
asked May 26 at 3:16
tphtph
384 bronze badges
384 bronze badges
2
$begingroup$
For the record, if you're just getting started making apps, credit card handling is not the best place, given the immense legal/contractual requirements/standards around them. Most businesses will want to outsource card handling.
$endgroup$
– jpmc26
May 26 at 15:52
$begingroup$
I understand that, I was simply testing my abilities to write a simple checksum algorithm. I plan on writing a program to solve Rubix cubes soon as my first big project.
$endgroup$
– tph
May 26 at 16:23
add a comment |
2
$begingroup$
For the record, if you're just getting started making apps, credit card handling is not the best place, given the immense legal/contractual requirements/standards around them. Most businesses will want to outsource card handling.
$endgroup$
– jpmc26
May 26 at 15:52
$begingroup$
I understand that, I was simply testing my abilities to write a simple checksum algorithm. I plan on writing a program to solve Rubix cubes soon as my first big project.
$endgroup$
– tph
May 26 at 16:23
2
2
$begingroup$
For the record, if you're just getting started making apps, credit card handling is not the best place, given the immense legal/contractual requirements/standards around them. Most businesses will want to outsource card handling.
$endgroup$
– jpmc26
May 26 at 15:52
$begingroup$
For the record, if you're just getting started making apps, credit card handling is not the best place, given the immense legal/contractual requirements/standards around them. Most businesses will want to outsource card handling.
$endgroup$
– jpmc26
May 26 at 15:52
$begingroup$
I understand that, I was simply testing my abilities to write a simple checksum algorithm. I plan on writing a program to solve Rubix cubes soon as my first big project.
$endgroup$
– tph
May 26 at 16:23
$begingroup$
I understand that, I was simply testing my abilities to write a simple checksum algorithm. I plan on writing a program to solve Rubix cubes soon as my first big project.
$endgroup$
– tph
May 26 at 16:23
add a comment |
2 Answers
2
active
oldest
votes
$begingroup$
For making your code faster, we can choose more Pythonic ways in some parts of it.
First, let's take a look at convertToList
function. The goal of this function is to split digits of a number to a list of int
values. I want to follow your algorithm and do this by converting the number to the str
and then splitting it.
I want to do this by "List Comprehension":
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
We made some changes here. First of all, I changed the name of the function from camelCase (convertToList
) to snake_case (convert_to_list
) because according to the Python style guide, it is the better way. You can read more about the Python style guide at PEP8.
Next change is I replaced all your code with a single line list comprehension. The first advantage, we have written less code. Less code means probably fewer bugs.
But the second advantage here is this code is so much faster. How much? I have written a simple script for it; the result is that on average, the second version is 1.7 times faster.
Now let's move on and take another look at addDigits
function. I want to choose pythonic way again here:
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
Like the previous time, I changed the name. I think this name is clearer and everyone could tell what this code does.
For converting the number to a list of digits, I used convert_to_list
function instead of writing the whole code again. We are using functions to avoiding duplication, so I think it's a bad idea to write the same code here. For calculating the sum of digits in a list, I strongly recommend you that always use built-in function sum
. It's faster, you don't need to write new code and every Python programmer can tell what you are doing at first glance. This code is somehow 1.2 times faster than previous.
Now let's go to the beginning of your code. We want to parse command line parameters and be sure that the input is correct.
Even though we only call those codes once, I think it is a great favor to the readability of the code to move those lines in a separate function.
from re import search
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
I separated your code into two functions. The first function gets argv
list as an input parameter and if its length is equal to 2, returns the second parameter. Else, it will raise an Exception. There are a lot of people out there who are against exceptions and I agree with most of their reasons. But when we want our program to stop when a bad input came in, I think using exceptions is the best way.
The second function simply uss search
function of re
module. It checks that the input string only contains 16 digits. If that assumption will be true, True
will be returned. Otherwise, the False
value is what you get.
Now you can change your main
function like this:
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
What we do is if card number is not a 16-digit number, code in the last else
will execute. Otherwise, codes in the first if
will run. That looks nicer to me.
So now let's go to the last function, the cardCheck
.
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
There were some problems in your code that I tried to fix. First, you don't need to put if
conditions in parentheses. In python, don't need means you should not.
Second, if you run the same code in if
and else
; you should take that part of code away from if
statement. That is what I did with card_check(digits_list, count + 1)
line. That line was repeated needlessly.
In the end, for logical operations, you should use operators like and
and or
, not &
and |
.
Here is the full code. I hope that helps you:
from re import search
import sys
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
if __name__ == '__main__':
main()
$endgroup$
add a comment |
$begingroup$
Regarding the cardCheck function: Its main purpose is to help you determine if a card number is valid, using the Luhn algorithm. So why have it return a sum, rather than a True or False? I can't imagine a scenario where you'd want to call that function and NOT want to do the sum % 10 == 0
check after. So include that in the function, that seems to be a core part of its job. The main function can then be more explicit when using it:
valid = cardCheck(number):
if valid:
print('Valid card!')
Also, there's no reason to make this function recursive. That seems to only make it harder to understand. Compare with this variant that iterates over the digits instead:
def cardCheck(digit_list):
if len(digit_list) != 16:
return False
total = 0
for i, digit in enumerate(digit_list):
if i % 2 == 0:
digit *= 2
if digit >= 10:
digit -= 9
total += digit
return total % 10 == 0
I used a trick here to replace the digit sum: If the digit sum is needed, that will be on a number between 10 and 18. The digit sum of those numbers is just 1 above the last digit, and that number can be more easily found by just subtracting 9. But whether you use that trick or not, the iterative version seems easier to understand and read.
$endgroup$
add a comment |
Your Answer
StackExchange.ifUsing("editor", function () {
StackExchange.using("externalEditor", function () {
StackExchange.using("snippets", function () {
StackExchange.snippets.init();
});
});
}, "code-snippets");
StackExchange.ready(function() {
var channelOptions = {
tags: "".split(" "),
id: "196"
};
initTagRenderer("".split(" "), "".split(" "), channelOptions);
StackExchange.using("externalEditor", function() {
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled) {
StackExchange.using("snippets", function() {
createEditor();
});
}
else {
createEditor();
}
});
function createEditor() {
StackExchange.prepareEditor({
heartbeatType: 'answer',
autoActivateHeartbeat: false,
convertImagesToLinks: false,
noModals: true,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
imageUploader: {
brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
allowUrls: true
},
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
});
}
});
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
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f221030%2fcredit-card-validator-using-luhn-algorithm%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
2 Answers
2
active
oldest
votes
2 Answers
2
active
oldest
votes
active
oldest
votes
active
oldest
votes
$begingroup$
For making your code faster, we can choose more Pythonic ways in some parts of it.
First, let's take a look at convertToList
function. The goal of this function is to split digits of a number to a list of int
values. I want to follow your algorithm and do this by converting the number to the str
and then splitting it.
I want to do this by "List Comprehension":
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
We made some changes here. First of all, I changed the name of the function from camelCase (convertToList
) to snake_case (convert_to_list
) because according to the Python style guide, it is the better way. You can read more about the Python style guide at PEP8.
Next change is I replaced all your code with a single line list comprehension. The first advantage, we have written less code. Less code means probably fewer bugs.
But the second advantage here is this code is so much faster. How much? I have written a simple script for it; the result is that on average, the second version is 1.7 times faster.
Now let's move on and take another look at addDigits
function. I want to choose pythonic way again here:
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
Like the previous time, I changed the name. I think this name is clearer and everyone could tell what this code does.
For converting the number to a list of digits, I used convert_to_list
function instead of writing the whole code again. We are using functions to avoiding duplication, so I think it's a bad idea to write the same code here. For calculating the sum of digits in a list, I strongly recommend you that always use built-in function sum
. It's faster, you don't need to write new code and every Python programmer can tell what you are doing at first glance. This code is somehow 1.2 times faster than previous.
Now let's go to the beginning of your code. We want to parse command line parameters and be sure that the input is correct.
Even though we only call those codes once, I think it is a great favor to the readability of the code to move those lines in a separate function.
from re import search
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
I separated your code into two functions. The first function gets argv
list as an input parameter and if its length is equal to 2, returns the second parameter. Else, it will raise an Exception. There are a lot of people out there who are against exceptions and I agree with most of their reasons. But when we want our program to stop when a bad input came in, I think using exceptions is the best way.
The second function simply uss search
function of re
module. It checks that the input string only contains 16 digits. If that assumption will be true, True
will be returned. Otherwise, the False
value is what you get.
Now you can change your main
function like this:
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
What we do is if card number is not a 16-digit number, code in the last else
will execute. Otherwise, codes in the first if
will run. That looks nicer to me.
So now let's go to the last function, the cardCheck
.
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
There were some problems in your code that I tried to fix. First, you don't need to put if
conditions in parentheses. In python, don't need means you should not.
Second, if you run the same code in if
and else
; you should take that part of code away from if
statement. That is what I did with card_check(digits_list, count + 1)
line. That line was repeated needlessly.
In the end, for logical operations, you should use operators like and
and or
, not &
and |
.
Here is the full code. I hope that helps you:
from re import search
import sys
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
if __name__ == '__main__':
main()
$endgroup$
add a comment |
$begingroup$
For making your code faster, we can choose more Pythonic ways in some parts of it.
First, let's take a look at convertToList
function. The goal of this function is to split digits of a number to a list of int
values. I want to follow your algorithm and do this by converting the number to the str
and then splitting it.
I want to do this by "List Comprehension":
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
We made some changes here. First of all, I changed the name of the function from camelCase (convertToList
) to snake_case (convert_to_list
) because according to the Python style guide, it is the better way. You can read more about the Python style guide at PEP8.
Next change is I replaced all your code with a single line list comprehension. The first advantage, we have written less code. Less code means probably fewer bugs.
But the second advantage here is this code is so much faster. How much? I have written a simple script for it; the result is that on average, the second version is 1.7 times faster.
Now let's move on and take another look at addDigits
function. I want to choose pythonic way again here:
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
Like the previous time, I changed the name. I think this name is clearer and everyone could tell what this code does.
For converting the number to a list of digits, I used convert_to_list
function instead of writing the whole code again. We are using functions to avoiding duplication, so I think it's a bad idea to write the same code here. For calculating the sum of digits in a list, I strongly recommend you that always use built-in function sum
. It's faster, you don't need to write new code and every Python programmer can tell what you are doing at first glance. This code is somehow 1.2 times faster than previous.
Now let's go to the beginning of your code. We want to parse command line parameters and be sure that the input is correct.
Even though we only call those codes once, I think it is a great favor to the readability of the code to move those lines in a separate function.
from re import search
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
I separated your code into two functions. The first function gets argv
list as an input parameter and if its length is equal to 2, returns the second parameter. Else, it will raise an Exception. There are a lot of people out there who are against exceptions and I agree with most of their reasons. But when we want our program to stop when a bad input came in, I think using exceptions is the best way.
The second function simply uss search
function of re
module. It checks that the input string only contains 16 digits. If that assumption will be true, True
will be returned. Otherwise, the False
value is what you get.
Now you can change your main
function like this:
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
What we do is if card number is not a 16-digit number, code in the last else
will execute. Otherwise, codes in the first if
will run. That looks nicer to me.
So now let's go to the last function, the cardCheck
.
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
There were some problems in your code that I tried to fix. First, you don't need to put if
conditions in parentheses. In python, don't need means you should not.
Second, if you run the same code in if
and else
; you should take that part of code away from if
statement. That is what I did with card_check(digits_list, count + 1)
line. That line was repeated needlessly.
In the end, for logical operations, you should use operators like and
and or
, not &
and |
.
Here is the full code. I hope that helps you:
from re import search
import sys
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
if __name__ == '__main__':
main()
$endgroup$
add a comment |
$begingroup$
For making your code faster, we can choose more Pythonic ways in some parts of it.
First, let's take a look at convertToList
function. The goal of this function is to split digits of a number to a list of int
values. I want to follow your algorithm and do this by converting the number to the str
and then splitting it.
I want to do this by "List Comprehension":
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
We made some changes here. First of all, I changed the name of the function from camelCase (convertToList
) to snake_case (convert_to_list
) because according to the Python style guide, it is the better way. You can read more about the Python style guide at PEP8.
Next change is I replaced all your code with a single line list comprehension. The first advantage, we have written less code. Less code means probably fewer bugs.
But the second advantage here is this code is so much faster. How much? I have written a simple script for it; the result is that on average, the second version is 1.7 times faster.
Now let's move on and take another look at addDigits
function. I want to choose pythonic way again here:
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
Like the previous time, I changed the name. I think this name is clearer and everyone could tell what this code does.
For converting the number to a list of digits, I used convert_to_list
function instead of writing the whole code again. We are using functions to avoiding duplication, so I think it's a bad idea to write the same code here. For calculating the sum of digits in a list, I strongly recommend you that always use built-in function sum
. It's faster, you don't need to write new code and every Python programmer can tell what you are doing at first glance. This code is somehow 1.2 times faster than previous.
Now let's go to the beginning of your code. We want to parse command line parameters and be sure that the input is correct.
Even though we only call those codes once, I think it is a great favor to the readability of the code to move those lines in a separate function.
from re import search
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
I separated your code into two functions. The first function gets argv
list as an input parameter and if its length is equal to 2, returns the second parameter. Else, it will raise an Exception. There are a lot of people out there who are against exceptions and I agree with most of their reasons. But when we want our program to stop when a bad input came in, I think using exceptions is the best way.
The second function simply uss search
function of re
module. It checks that the input string only contains 16 digits. If that assumption will be true, True
will be returned. Otherwise, the False
value is what you get.
Now you can change your main
function like this:
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
What we do is if card number is not a 16-digit number, code in the last else
will execute. Otherwise, codes in the first if
will run. That looks nicer to me.
So now let's go to the last function, the cardCheck
.
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
There were some problems in your code that I tried to fix. First, you don't need to put if
conditions in parentheses. In python, don't need means you should not.
Second, if you run the same code in if
and else
; you should take that part of code away from if
statement. That is what I did with card_check(digits_list, count + 1)
line. That line was repeated needlessly.
In the end, for logical operations, you should use operators like and
and or
, not &
and |
.
Here is the full code. I hope that helps you:
from re import search
import sys
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
if __name__ == '__main__':
main()
$endgroup$
For making your code faster, we can choose more Pythonic ways in some parts of it.
First, let's take a look at convertToList
function. The goal of this function is to split digits of a number to a list of int
values. I want to follow your algorithm and do this by converting the number to the str
and then splitting it.
I want to do this by "List Comprehension":
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
We made some changes here. First of all, I changed the name of the function from camelCase (convertToList
) to snake_case (convert_to_list
) because according to the Python style guide, it is the better way. You can read more about the Python style guide at PEP8.
Next change is I replaced all your code with a single line list comprehension. The first advantage, we have written less code. Less code means probably fewer bugs.
But the second advantage here is this code is so much faster. How much? I have written a simple script for it; the result is that on average, the second version is 1.7 times faster.
Now let's move on and take another look at addDigits
function. I want to choose pythonic way again here:
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
Like the previous time, I changed the name. I think this name is clearer and everyone could tell what this code does.
For converting the number to a list of digits, I used convert_to_list
function instead of writing the whole code again. We are using functions to avoiding duplication, so I think it's a bad idea to write the same code here. For calculating the sum of digits in a list, I strongly recommend you that always use built-in function sum
. It's faster, you don't need to write new code and every Python programmer can tell what you are doing at first glance. This code is somehow 1.2 times faster than previous.
Now let's go to the beginning of your code. We want to parse command line parameters and be sure that the input is correct.
Even though we only call those codes once, I think it is a great favor to the readability of the code to move those lines in a separate function.
from re import search
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
I separated your code into two functions. The first function gets argv
list as an input parameter and if its length is equal to 2, returns the second parameter. Else, it will raise an Exception. There are a lot of people out there who are against exceptions and I agree with most of their reasons. But when we want our program to stop when a bad input came in, I think using exceptions is the best way.
The second function simply uss search
function of re
module. It checks that the input string only contains 16 digits. If that assumption will be true, True
will be returned. Otherwise, the False
value is what you get.
Now you can change your main
function like this:
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
What we do is if card number is not a 16-digit number, code in the last else
will execute. Otherwise, codes in the first if
will run. That looks nicer to me.
So now let's go to the last function, the cardCheck
.
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
There were some problems in your code that I tried to fix. First, you don't need to put if
conditions in parentheses. In python, don't need means you should not.
Second, if you run the same code in if
and else
; you should take that part of code away from if
statement. That is what I did with card_check(digits_list, count + 1)
line. That line was repeated needlessly.
In the end, for logical operations, you should use operators like and
and or
, not &
and |
.
Here is the full code. I hope that helps you:
from re import search
import sys
def get_input_from_cmd(args_list):
if len(args_list) != 2:
raise Exception("You should enter a 16-digit number as input argument")
return args_list[1]
def is_input_valid(input_str):
return bool(search(r"d{16}", input_str))
def convert_to_list(num):
result = [int(x) for x in str(num)]
return result
def sum_of_digits(number):
num_list = convert_to_list(number)
return sum(num_list)
def card_check(digits_list, count=0):
result = 0
if count % 2 == 0:
digits_list[count] *= 2
if digits_list[count] >= 10:
digits_list[count] = sum_of_digits(digits_list[count])
if count < 15:
card_check(digits_list, count + 1)
else:
return 0
result += sum(digits_list)
return result
def main():
input_string = get_input_from_cmd(sys.argv)
if is_input_valid(input_string):
digits_list = convert_to_list(input_string)
card_checking_sum = card_check(digits_list)
if card_checking_sum % 10 == 0:
print('Valid Card!')
else:
print('Invalid Card!')
else:
print("Invalid Card number")
if __name__ == '__main__':
main()
edited May 30 at 11:49
Toby Speight
31.1k7 gold badges45 silver badges135 bronze badges
31.1k7 gold badges45 silver badges135 bronze badges
answered May 26 at 6:26
Mr AlihoseinyMr Alihoseiny
3548 bronze badges
3548 bronze badges
add a comment |
add a comment |
$begingroup$
Regarding the cardCheck function: Its main purpose is to help you determine if a card number is valid, using the Luhn algorithm. So why have it return a sum, rather than a True or False? I can't imagine a scenario where you'd want to call that function and NOT want to do the sum % 10 == 0
check after. So include that in the function, that seems to be a core part of its job. The main function can then be more explicit when using it:
valid = cardCheck(number):
if valid:
print('Valid card!')
Also, there's no reason to make this function recursive. That seems to only make it harder to understand. Compare with this variant that iterates over the digits instead:
def cardCheck(digit_list):
if len(digit_list) != 16:
return False
total = 0
for i, digit in enumerate(digit_list):
if i % 2 == 0:
digit *= 2
if digit >= 10:
digit -= 9
total += digit
return total % 10 == 0
I used a trick here to replace the digit sum: If the digit sum is needed, that will be on a number between 10 and 18. The digit sum of those numbers is just 1 above the last digit, and that number can be more easily found by just subtracting 9. But whether you use that trick or not, the iterative version seems easier to understand and read.
$endgroup$
add a comment |
$begingroup$
Regarding the cardCheck function: Its main purpose is to help you determine if a card number is valid, using the Luhn algorithm. So why have it return a sum, rather than a True or False? I can't imagine a scenario where you'd want to call that function and NOT want to do the sum % 10 == 0
check after. So include that in the function, that seems to be a core part of its job. The main function can then be more explicit when using it:
valid = cardCheck(number):
if valid:
print('Valid card!')
Also, there's no reason to make this function recursive. That seems to only make it harder to understand. Compare with this variant that iterates over the digits instead:
def cardCheck(digit_list):
if len(digit_list) != 16:
return False
total = 0
for i, digit in enumerate(digit_list):
if i % 2 == 0:
digit *= 2
if digit >= 10:
digit -= 9
total += digit
return total % 10 == 0
I used a trick here to replace the digit sum: If the digit sum is needed, that will be on a number between 10 and 18. The digit sum of those numbers is just 1 above the last digit, and that number can be more easily found by just subtracting 9. But whether you use that trick or not, the iterative version seems easier to understand and read.
$endgroup$
add a comment |
$begingroup$
Regarding the cardCheck function: Its main purpose is to help you determine if a card number is valid, using the Luhn algorithm. So why have it return a sum, rather than a True or False? I can't imagine a scenario where you'd want to call that function and NOT want to do the sum % 10 == 0
check after. So include that in the function, that seems to be a core part of its job. The main function can then be more explicit when using it:
valid = cardCheck(number):
if valid:
print('Valid card!')
Also, there's no reason to make this function recursive. That seems to only make it harder to understand. Compare with this variant that iterates over the digits instead:
def cardCheck(digit_list):
if len(digit_list) != 16:
return False
total = 0
for i, digit in enumerate(digit_list):
if i % 2 == 0:
digit *= 2
if digit >= 10:
digit -= 9
total += digit
return total % 10 == 0
I used a trick here to replace the digit sum: If the digit sum is needed, that will be on a number between 10 and 18. The digit sum of those numbers is just 1 above the last digit, and that number can be more easily found by just subtracting 9. But whether you use that trick or not, the iterative version seems easier to understand and read.
$endgroup$
Regarding the cardCheck function: Its main purpose is to help you determine if a card number is valid, using the Luhn algorithm. So why have it return a sum, rather than a True or False? I can't imagine a scenario where you'd want to call that function and NOT want to do the sum % 10 == 0
check after. So include that in the function, that seems to be a core part of its job. The main function can then be more explicit when using it:
valid = cardCheck(number):
if valid:
print('Valid card!')
Also, there's no reason to make this function recursive. That seems to only make it harder to understand. Compare with this variant that iterates over the digits instead:
def cardCheck(digit_list):
if len(digit_list) != 16:
return False
total = 0
for i, digit in enumerate(digit_list):
if i % 2 == 0:
digit *= 2
if digit >= 10:
digit -= 9
total += digit
return total % 10 == 0
I used a trick here to replace the digit sum: If the digit sum is needed, that will be on a number between 10 and 18. The digit sum of those numbers is just 1 above the last digit, and that number can be more easily found by just subtracting 9. But whether you use that trick or not, the iterative version seems easier to understand and read.
answered May 26 at 7:51
SedsarqSedsarq
3496 bronze badges
3496 bronze badges
add a comment |
add a comment |
Thanks for contributing an answer to Code Review Stack Exchange!
- Please be sure to answer the question. Provide details and share your research!
But avoid …
- Asking for help, clarification, or responding to other answers.
- Making statements based on opinion; back them up with references or personal experience.
Use MathJax to format equations. MathJax reference.
To learn more, see our tips on writing great answers.
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
Required, but never shown
StackExchange.ready(
function () {
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f221030%2fcredit-card-validator-using-luhn-algorithm%23new-answer', 'question_page');
}
);
Post as a guest
Required, but never shown
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
Required, but never shown
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
Required, but never shown
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
Post as a guest
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
Required, but never shown
2
$begingroup$
For the record, if you're just getting started making apps, credit card handling is not the best place, given the immense legal/contractual requirements/standards around them. Most businesses will want to outsource card handling.
$endgroup$
– jpmc26
May 26 at 15:52
$begingroup$
I understand that, I was simply testing my abilities to write a simple checksum algorithm. I plan on writing a program to solve Rubix cubes soon as my first big project.
$endgroup$
– tph
May 26 at 16:23