Coding style
First, have a read through the Python style guide, PEP 8.
There's a lot of inconsistency in your coding style, and with the style guide, you should be able to remove all that. It will then also become consistent in style with a lot of existing Python code.
With that in hand, you can fix the following:
often, single space instead of double space.
Not if x is True
, but if x is True
.
surround operators with a space, except as keyword arguments.
Not total =0
, but total = 0
.
Not sum=0
, but sum = 0
.
leave a whitespace after a comma.
Not def doubledigit(n,x) :
, but def doubledigit(n, x):
(note: that also removed a double space, and remove the space before the :
.)
remove parentheses from return
statements and in conditions.
Not: return (sum)
, but return sum
.
Not: if (n % 10 == 1 ):
, but if n % 10 == 1:
avoid putting things on the same line after a colon.
Not else: sum = doubledigit
, but
else:
sum = doubledigit
If an argument in a condition (eg, if-statement) has a truthy or falsy value, or is simply True
or False
, don't compare it to anything (exceptions exist, but not so much here), but use the value itself.
Not: if x is True:
but if x:
Remove unnecessary line breaks for code that belongs together, such as in your if blocks. Do insert an extra line break after a function ends.
Thus: an extra line break after return sum
and return total
.
But remove the line breaks after print("the check digit is 8")
and similar lines, because the next line definitely belongs to that if-clause.
Avoid overwriting built-int variables and functions.
Not: def sum():
, but e.g. def calc_total()
and for the variable named sum
, use a different name like total
.
(I've used calc_total()
here to avoid clashes with the variable named total
, although such double naming will often work. It is, however, also a good idea to use a verb in your function name, to show it does something (calc
is a bit generic, perhaps).
Here's the list with built-in functions
Use clearly named variables. n
is just about acceptable (assuming it's short for number
, but x
is too generic. I don't know the algorithm well enough to come up with an appropriate name, but something like double_flag
could be a decent name (tad long perhaps).
The same holds for function names: findx
is rather generic, and from the name, I have no idea what we're trying to find here (relevant joke).
User input
Have a look at your input statements. There is a direct conversion to integer, which will cause a crash if someone accidentally enters, say 'a'. A crash is better than using, e.g., the value 97, but ideally, some user feedback is nice. Consider:
while True:
n = input("Enter your digit: ")
try:
n = int(n)
break
except ValueError:
print("Please enter a number", file=sys.stderr)
and similar for prop
.
This costs more effort (and code), but when dealing with user interaction, there'll be a lot of extra code (never think "this will never happen for this code", because it will).
(Add an import sys
at the top of your code, for the output to standard error.)
Note: I've re-used n
here, first as the result of input()
(being a str
), then promptly assigning it to its actual, integer value. Since these lines are close together, and within the same while
block, I don't feel this is an issue, but usually, it's better to use different variable names. E.g., answer = input(...)
and then n = int(answer)
. See also Konrad Rudolph's comment below.
For the true or false input, avoid the ast.literal_eval
. Just compare directly to "True"
or "False"
input strings, and set x
accordingly:
while True:
x = input("Enter True or False: ").lower()
if x in ['true', 'false']:
x = True if x == 'true' else False
break
total = total + doubledigit(n, x)
(I've taken the liberty to allow "true", "falSe" etc as input as well.)
I've added a short, ternary if-statement: x = True if x == 'true' else False
. If that's new and confusing, it's exactly the same as:
if x == 'true':
x = True
else:
x = False
Alternatively, since a comparison yields a True
or False
value, you can also simply use:
x = x == 'true'
as Konrad Rudolph in the comments points out.
If chain
Your set of if statements all look remarkably similar. And if you check the operations inside the if statement and following lines, you may notice that while one set of digits increase, the other set decreases.
In fact, checkdigit = 10 - n % 10
, and with that, all your if statements can be removed, and replaced with just this:
checkdigit = 10 - n % 10
print("the check digit is", checkdigit)
n += checkdigit
Of course, there is the n % 10 == 0
case, where the message changes, and no addition takes place. So:
checkdigit = 10 - n % 10
if checkdigit == 10:
print("valid checksum:")
else:
print("the check digit is", checkdigit)
n += checkdigit
Using Python code as script or module
Your call to findx
is at the top level of your code (at the bottom). That is, if you ever import your script (say, import luhn
), the code will immediately execute. That is fine as a script (run as python luhn.py
), but rarely when importing.
To avoid, a slightly fancy trick is applied, that'll you find in lots of places: we can check whether a file is imported or executed as a script. In the latter case, the special variable __name__
takes the value "__main__"
(a string). Testing for that in an if statement avoids running the code when imported:
if __name__ == "__main__":
print(findx(sum()))
Now you can import luhn
without a problem, and then manually run luhn.findx(sum())
at your leisure (or, for example, just luhn.sum()
).
Nested call
You call findx
inside a print statement, and you calculate sum
inside the call to findx
. For clarity, it is nicer to separate this (there are cases where this is fine, but in general, separating them makes it easier to follow the program flow):
total = sum()
x = findx(total)
print(x)
Comments and doc strings
Python has the great ability to allow documentation inside the code, with so-called docstrings. Ideally, every script, function etc. has a docstring. Sometimes short, sometimes long. It describes what the function does, what inputs it expects, what errors can be raised. I haven't really bothered here with it, but it could be good to have them.
It would look a bit like:
def doubledigit(n, x):
"""Calculate the sum for double digits
Calculate the sum for double digits, with the following algorithm:
<brief description of algorithm>
Inputs:
- n: an integer
- x: True or False
Returns:
The sum of double digits
"""
Have a read through the conventions used, or search around for more about Python docstrings: they are immensely useful if you start writing larger scripts, modules, packages, since they automatically provide documentation for both yourself (in a months time) and other users.
Algorithm
The one thing I haven't done, is verifying the correctness of the actual algorithm. Creating some automated tests (unit tests) would be good, but that's a step advanced.
With all the above applied, and some other minor fixes (e.g., sum = 0
is unnecessary, since it gets assigned either way in the lines below), my version of your code now looks as follows:
import sys
def doubledigit(n, double_flag):
if double_flag:
doubledigit = 2 * n
if doubledigit >= 10 :
total = 1 + doubledigit % 10
else:
total = doubledigit
else:
total = n
return total
def calc_total():
total = 0
while True:
prop = input("Enter how many digits you want: ")
try:
prop = int(prop)
break
except ValueError:
print("Please enter a number", file=sys.stderr)
for _ in range(0, prop):
while True:
n = input("Enter your digit: ")
try:
n = int(n)
break
except ValueError:
print("Please enter a number", file=sys.stderr)
while True:
double_flag = input("Enter True or False: ").lower()
if double_flag in ['true', 'false']:
double_flag = double_flag == 'true'
break
total += doubledigit(n, double_flag)
return total
def findx(n):
checkdigit = 10 - n % 10
if n == 10:
print("Valid checksum")
else:
print("The check digit is", checkdigit)
n += checkdigit
return n
if __name__ == "__main__":
total = calc_total()
x = findx(total)
print(x)