find if a gtin is valid

Question

I was given that assignment recently as part of a coding interview.

The idea is to find if a gtin is valid or not valid.

The requirements are the following:

• make sure the input is in a string format
• the length must be either 8,12,13,14,17 or 18

The algorithm is the following:

TL;DR: You need to multiply all odd positions by 3 and you have to remove the last position. For example if the GTIN is 13, you need to multiply the 12th position by 3, the 11th position by 1, the 12th position by 3 and so on.

If the GTIN is 8, you need to multiply the 7th position by 3, the 6th by 1, the 5th by 3 and so on.

Once done, you sum all the elements and you find the nearest equal or higher multiple of ten and you substract this from the sum you have obtained. The integer that you obtained should be equal to the last digit (13th position if the GTIN is 13).

If it is not, then the GTIN is an incorrect one.

Here is my code

 def gtin_check(gtin: str)-> str:
    if not isinstance(gtin, str) or len(gtin) not in (8,12,13,14,17,18):
        return 'Not a string'
    else:
        gtin_lst = [int(x) for x in gtin]
        #print("step 1 - give the gtin but as a list of integers",gtin_lst)
        gtin_lst.reverse()
        #print("step 2 reverse the gtin",gtin_lst)
        gtin_original = gtin_lst #saving the list to gtin_original
        #print("step 3 : gtin original still reverse",gtin_original)
        gtin_lst = gtin_original[1:] 
        #print("step 4 - gtin_lst starting from the first element",gtin_lst)
        gtin_original.reverse()
        #print("step 5 - gtin original without reverse , going back to normal ",gtin_original)

        # arriving at the algorithm part
        gtin_lst = [x*3 if i%2 == 0 else x for i,x in enumerate(gtin_lst)]
        #print('step 6 - multiplying by 3 of integers on odd position',gtin_lst)  
        som_gtin1 = sum(gtin_lst)
        #print('step 7 - gtins summation',som_gtin1)

        if (som_gtin1 % 10):
            som_gtin2 = som_gtin1 + (10 - som_gtin1 % 10) # (10 // gcd(som_gtin1, 10)) *  som_gtin1 #int(round(som_gtin1, -1)) #som_gtin1 + (10 - som_gtin1 % 10)
        else:   
            som_gtin2 = som_gtin1 
        #print('step 8 : multiple of 10 instead of equal or higher summation',som_gtin2-som_gtin1)

        if (som_gtin2 - som_gtin1) == gtin_original[-1]:
            return True
        else:
            return False    

The code does what I want but I feel it is not pythonic enough. Any suggestions or insight on how can I write a lighter code are welcomed.

update 1:

Example 12345670 will put True. 12345678 will put False.

update 2:

Since MJ713's feedback, I've commented my code just to make sure you understand my code. Let me know if you have any issues in reading it. I've amended my code to fix the bugs, MJ713 mentionned.

Answer 1

I'll start from the top and work my way down to the bottom.

---

---

 def gtin_check(gtin: str)-> str:

Your type-hinting is incorrect here. Except in cases of invalid input, you are returning a boolean, not a string. Which leads me to...

    if not isinstance(gtin, str) or len(gtin) not in (8,12,13,14,17,18):
        return 'Not a string'

In general it is a bad idea to mix two completely different return types for one function, unless there is some pressing reason to do so. The mixing creates more work for whoever is calling your function, since they have to handle both types. (Null value/None is probably the most common exception to this rule; sometimes it's appropriate to return None, e.g. if a function that is searching for a single specific object or value doesn't find one.)

To get rid of the string return value, I would suggest one of these alternatives:

• If the calling function really needs to know the difference between an input that had a bad check digit, and an input that was invalid in a "stronger" way: either raise an exception, or make your return type an enum with three possible values (e.g. VALID, INVALID, and MALFORMED).
• If the calling function only needs to know whether an input was a valid GTIN or not: just return False.

    if not isinstance(gtin, str) or len(gtin) not in (8,12,13,14,17,18):
        return False

Now let's talk about performance. in / not in checks are generally faster on sets than they are on lists or tuples. This is because sets do clever things with hashing (\$O(1)\$), while with lists and tuples you have to go down the line and check every value in order (\$O(n)\$). So let's use a set instead.

    if not isinstance(gtin, str) or len(gtin) not in {8,12,13,14,17,18}:
        return False

While we're doing these other checks, it would also be good to use .isdecimal() to check that the input string is numeric. Right now we don't have any code to catch non-numeric inputs like 'asdfghjk', so we get a slightly ugly error (ValueError: invalid literal for int() with base 10: 'a').

    if (not isinstance(gtin, str)
            or not gtin.isdecimal()
            or len(gtin) not in {8,12,13,14,17,18}):
        return False

The line was getting a bit long, so I added some line breaks. See the PEP 8 document for guidelines on when and how to use line breaks.

---

---

        gtin_lst = [int(x) for x in gtin]
        #print("step 1 - give the gtin but as a list of integers",gtin_lst)
        gtin_lst.reverse()
        #print("step 2 reverse the gtin",gtin_lst)
        gtin_original = gtin_lst #saving the list to gtin_original
        #print("step 3 : gtin original still reverse",gtin_original)
        gtin_lst = gtin_original[1:] 
        #print("step 4 - gtin_lst starting from the first element",gtin_lst)
        gtin_original.reverse()

This section is confusing - you're putting the original value in a new variable and the altered value in the original variable, not to mention reversing a reversal. It would be easier to understand if you left gtin_lst intact and used negative indexes to extract the subset you need:

        gtin_lst = [int(x) for x in gtin]
        gtin_lst_without_check_digit = gtin_lst[:-1]
        gtin_lst_without_check_digit.reverse()

---

---

        # arriving at the algorithm part
        gtin_lst = [x*3 if i%2 == 0 else x for i,x in enumerate(gtin_lst)]

It is more "pythonic" to avoid == 0 in conditions, and instead use the inherent "falsiness" of 0.

x*3 if not i%2 else x

I see you have already done this in another part of the code: if (som_gtin1 % 10):.

---

---

        if (som_gtin1 % 10):
            som_gtin2 = som_gtin1 + (10 - som_gtin1 % 10) # (10 // gcd(som_gtin1, 10)) *  som_gtin1 #int(round(som_gtin1, -1)) #som_gtin1 + (10 - som_gtin1 % 10)
        else:   
            som_gtin2 = som_gtin1 

The logic here is solid, but this seems like a good place to talk about naming.

In general, naming variables by the pattern foo1, foo2, etc. is a code smell. If you have more than one of the same kind of thing, why aren't they in some kind of list or collection? And if they aren't the same kind of thing, why do they have the same name? There are exceptions, but I would say this is a bad idea nine times out of ten.

In this case, som_gtin1 and som_gtin2 aren't the same kind of thing.

• som_gtin1 is a sum of some numbers. (Is "som" instead of "sum" a typo, or are you deliberately using a non-English word?)
• som_gtin2 is som_gtin1 rounded to the nearest highest 10. In other words, it is based on a sum, but it is not a sum itself.

Also, since the function is named gtin_check, we already know that everything we're doing is related to GTINs somehow. So adding _gtin to the variable names doesn't really give the reader any additional information.

Picking accurate and descriptive names throughout your code will make it easier to read and understand. You will sometimes hear people describe this kind of code as "self-documenting", because it does not need nearly as many comments or external documents.

        if (digits_sum % 10):
            uprounded_sum = digits_sum + (10 - digits_sum % 10)
        else:   
            uprounded_sum = digits_sum

Let me be clear: I'm not claiming that my names are perfect either. There's an old joke that "there are only two hard problems in computer science: cache invalidation and naming things."

---

---

        if (som_gtin2 - som_gtin1) == gtin_original[-1]:
            return True
        else:
            return False

I had a computer science professor in college who specifically warned us not to use this construction. So naturally, I didn't notice it until an hour after I finished writing the rest of this answer. 🙂

As my professor pointed out, any logic of the form "if boolean value, return true, else return false" can be reduced to "return boolean value".

        return ((som_gtin2 - som_gtin1) == gtin_original[-1])

The wrapping parentheses aren't strictly necessary, but I think they make the code clearer.

---

---

Putting it all together

def gtin_check(gtin: str) -> bool:
    """Returns whether the input is a well-formed GTIN string."""
    # Check for obvious problems
    if (not isinstance(gtin, str)
            or not gtin.isdecimal()
            or len(gtin) not in (8,12,13,14,17,18)):
        return False
    else:
        # Compute correct "check digit" and compare to input's digit
        original_digits = [int(x) for x in gtin]
        digits_without_check_digit = original_digits[:-1]

        digits_without_check_digit.reverse()
        multiplied_digits = [x*3 if not i%2 else x
                             for i,x
                             in enumerate(digits_without_check_digit)]
        digits_sum = sum(multiplied_digits)

        if (digits_sum % 10):
            uprounded_sum = digits_sum + (10 - digits_sum % 10)
        else:   
            uprounded_sum = digits_sum
        expected_check_digit = uprounded_sum - digits_sum

        return (original_digits[-1] == expected_check_digit)