6
\$\begingroup\$

I've implemented Luhn checksum in Python version 3. I want your comments and opinion about my code. I know it's embarrassing code and that it doesn't follow python best practices but it works!.

The x in doubledigit parameter means that if the boolean is False, the index of the number is odd so we don't have to double it, if it is True it means the index is even, so we double it.

import ast

def  doubledigit(n,x) :

    if x is  True:

        doubledigit = n *2
        sum=0
        if doubledigit >= 10 :
            sum = 1 + doubledigit % 10
        else: sum = doubledigit
    else:
        sum = n


    return (sum)
def sum ():
    total =0
    prop = int(input("enter how many digits you want: " ))
    for i in range (0,prop):
        n = int(input("Enter your digit: " ))
        x = input("Enter True or False: ")
        total = total+doubledigit(n,ast.literal_eval(x))

    return total
def findx (n):
    if (n % 10 == 1 ):
        print("the check digit is 9")

        n += 9
    if (n % 10 == 2 ):
        print("the check digit is 8")

        n += 8
    if (n % 10 == 3 ):
        print("the check digit is 7")
        n += 7
    if (n % 10 == 4 ):
        print("the check digit is 6")

        n += 6
    if (n % 10 == 5 ):
        print("the check digit is 5")

        n += 5
    if (n % 10 == 6 ):
        print("the check digit is 4")

        n += 4
    if (n % 10 == 7 ):
        print("the check digit is 3")

        n += 3
    if (n % 10 == 8 ):
        print("the check digit is 2")

        n += 2
    if (n % 10 == 9 ):
        print("the check digit is 1")

        n += 1
    if (n % 10 == 0):
        print("vaild checksum")
    return n
print(findx(sum()))    
\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to CodeReview.SE! It seems like there is a formatting error in your code. When I run it, I get "SyntaxError: 'return' outside function". \$\endgroup\$ – SylvainD Jun 27 '18 at 13:33
  • \$\begingroup\$ @BillalBEGUERADJI really found some interesting topics in your webiste can you please provide me your e-mail ]or anything i really wanna talk to you.Thanks a lot \$\endgroup\$ – Omaroo Baniessa Jun 27 '18 at 13:47
  • \$\begingroup\$ Could be, but since this is Python, that leaves your code broken. And we won't review broken code (as per the help center). Remove the code, copy-paste the original into the question, select the code in the question and hit Ctrl+K. That should take care of the indentation for you. And if that doesn't fix it it's probably something minor that's easily spotted. It looks like all your returns are not enough indented. \$\endgroup\$ – Mast Jun 27 '18 at 13:47
  • \$\begingroup\$ @Josay I think there's IndentationError because this my first time posting a code here \$\endgroup\$ – Omaroo Baniessa Jun 27 '18 at 13:49
  • \$\begingroup\$ I've edited my code now it works fine \$\endgroup\$ – Omaroo Baniessa Jun 27 '18 at 13:59
8
\$\begingroup\$

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)
\$\endgroup\$
  • 2
    \$\begingroup\$ Great answer. I'd add that Python built-in functions such as sum shouldn't be used as variable names. \$\endgroup\$ – Will Da Silva Jun 27 '18 at 16:31
  • 1
    \$\begingroup\$ What’s the heck is x = True if x == 'true' else False?! → x = x == 'true'. But additionally: (1) don’t reuse a variable for multiple purposes, and especially don’t change its type! and (2) use proper names (actually I have no idea what x is doing; your suggested documentation doesn’t help either). Apart from that, excellent answer. \$\endgroup\$ – Konrad Rudolph Jun 27 '18 at 16:54
  • 1
    \$\begingroup\$ Shadowing a built-in function with a custom one that does something completely different seems pretty awful. And then the code could definitely profit from refactoring the nested loops into separate functions - particularly the input read functions. \$\endgroup\$ – Voo Jun 27 '18 at 17:32
  • \$\begingroup\$ I missed the built-in sum; nice catch!. I'll add that in due time as well. \$\endgroup\$ – 0 0 Jun 27 '18 at 20:14
  • \$\begingroup\$ @KonradRudolph I don't know what's more readable for setting x to True or False: there'll be a set of styles, including a full-out written if-else statement. If you mean by re-using the same variable and changing its type for the conversion of string x to integer / boolean x, I think here's that not an overall problem, since it's all very limited, and the type change is nearly instant. Can change for clarity, but I'll indeed need to find a proper name for x first. \$\endgroup\$ – 0 0 Jun 27 '18 at 20:19
0
\$\begingroup\$

To complement 9769953's excellent review, let me note a few issues they didn't touch upon. I'll start by reviewing your doubledigit function, before moving on to the checksum calculation itself.

The doubledigit function

I found your implementation of the doubledigit function rather excessively complicated. Here's how I'd rewrite it:

def doubledigit(digit, double_it):
    """
    If the second argument is true, doubles the given decimal digit and subtracts 9
    if the result is greater than 9. (This is equivalent to summing the digits of
    the doubled result.) Otherwise returns the given digit unchanged.
    """

    if double_it:
        digit = digit * 2
        if digit > 9:
            digit = digit - 9

    return digit

Besides simplifying the doubling calculation (as described e.g. on the Wikipedia article for the Luhn checksum), the most obvious change I made was replacing the parameter names n and x with more meaningful ones. In my opinion, using n / x as the name of a generic integer / float parameter to a mathematical function like factorial(n) or sin(x) is OK, but more descriptive names should be preferred. In any case, here the first parameter is not really an arbitrary integer (it has to be a single decimal digit) and the second parameter is definitely not an arbitrary number (it's a boolean flag!), so renaming them is definitely called for.

I also got rid of the sum variable, instead just modifying digit directly. This simplifies the code, since we don't have to do anything when double_it is false. I also removed the explicit is True comparison, as suggested by 9769953, and added a docstring to further explain the meaning of the parameters.

One thing I did not change is the meaning of the parameters, so the code above is a drop-in replacement for your implementation. In general, I find the use of boolean flags as function parameters to be a "code smell", and the use of a flag that is required for the function to actually do anything to be a particularly strong one. That said, sometimes smelly code can be OK, e.g. if it simplifies things for the caller. On that basis, one could argue for keeping the double_it flag here, especially if it was renamed to something like even_position so that the caller could at least pretend to be unaware of how the function actually works internally.

Still, my personal preference would be to remove the flag and just have the caller only call doubledigit for the digits that actually need doubling. That way, the function could be simplified to just:

def doubledigit(digit):
    """
    Doubles the given decimal digit and subtracts 9 if the result is greater than 9.
    (This is equivalent to summing the digits of the doubled result.)
    """

    digit = digit * 2
    if digit > 9:
        digit = digit - 9

    return digit

...and at this point, the code is so simple that (given that it's only called from one place, anyway) it might make more sense to get rid of the doubledigit function entirely and just include the equivalent lines of code directly in the checksum calculation loop. In fact, this is what I've done below.

The checksum calculation and user interface

As written, your code prompts the user to enter the number one digit at a time, and even requires them to manually indicate whether the digit is in an even or an odd position. Wouldn't it be much nicer if the user could just type in the whole number all at once?

In fact, why not make a function that takes an integer and calculates its Luhn checksum? Then we could handle the input e.g. like this (based on the input loop suggested by 9769953):

while True:
    n = input("Enter number with check digit: ")
    try:
        n = int(n)
        break
    except ValueError:
        print("Please enter a number.", file=sys.stderr)

error = luhn_checksum(n)

if error == 0:
    print("The check digit is correct.")
else:
    check_digit = n % 10
    correct = (check_digit - error) % 10  # in Python, this is always >= 0
    print("The correct check digit should be {0}.".format(correct))

(Of course, if we wanted our code to be also usable as a library, we should wrap this user interface code in an if __name__ == '__main__': block as suggested by 9769953. Also note that in Python x % 10 always yields a non-negative result, even if x is negative.)

Then we could write the checksum calculation e.g. like this:

def luhn_checksum(n):
    """
    Verifies the Luhn modulo-10 checksum for the number n. Returns 0 if the
    checksum is correct; otherwise returns the non-zero number that should be
    subtracted from the last digit of n (modulo 10) to make it correct.
    """

    # Sum the digits of the number in reverse order, doubling every second digit
    # and subtracting 9 if the doubled digit is greater than 9. (This is equivalent
    # to summing the digits of the doubled digit value separately.)

    digits = (int(d) for d in reversed(str(n)))
    total = 0
    for position, digit in enumerate(digits):
        if position % 2 == 1:
            digit *= 2
            if digit > 9: digit -= 9
        total += digit

    return total % 10

Of course, this is just an example implementation, and you're free to write your own code differently. But I included it as a demonstration that the problem can be simplified quite a bit.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.