4
\$\begingroup\$

I have a school assignment to allow a user to enter a GTIN and then validate that it is correct.

I have finished the code, but my teacher has told me I will get more marks if I can make it more efficient. I realise I could use a loop in lines 5-12, but am not completely sure how. Any feedback is welcome, but I'm really looking for ways to make the code more efficient.

GTIN=''
while True:
    GTIN=(input("please enter GTIN for product"))
    if GTIN.isnumeric() and len(GTIN)==8 and GTIN in open('read_it.txt').read():
        Num0=int(GTIN[0])*3
        Num1=int(GTIN[1])
        Num2=int(GTIN[2])*3
        Num3=int(GTIN[3])
        Num4=int(GTIN[4])*3
        Num5=int(GTIN[5])
        Num6=int(GTIN[6])*3
        Num7=int(GTIN[7])
        total2=(Num0+Num1+Num2+Num3+Num4+Num5+Num6+Num7)
        if total2 % 10 == 0:
            print(GTIN)
        else:
            print("Product Not Found")
\$\endgroup\$
5
\$\begingroup\$
  • You are opening and reading the file every time. Assuming the file doesn't change, this can be done once before the while True loop.
  • You could use a for loop to calculate the value of total2 but each of those calculations still need to happen. So it won't really change the efficiency of the code, it only changes how the logic is written.

You can write the code as follows to make it shorter:

GTIN=''
file_contents = open('read_it.txt').read()
while True:
    GTIN=(input("please enter GTIN for product"))
    if GTIN.isnumeric() and len(GTIN)==8 and GTIN in file_contents:
        total2 = 0
        for i, n in enumerate(GTIN):
            v = int(n)
            if i % 2 == 0:
                v = v * 3
            total2 += v
        if total2 % 10 == 0:
            print(GTIN)
        else:
            print("Product Not Found")

This uses the enumerate() function which allows you to loop over something while counting at the same time. So you'll get the first element of GTIN with the index 0, then the next element of GTIN with index 1 and so on.

The pattern is that if the index value is even (if i % 2 == 0:) then we multiple by 3. Otherwise we don't do that. At each step we add the calculated value to total2 and we then end up with the final value for total2.

If you don't want to use that then you can write it as:

GTIN=''
file_contents = open('read_it.txt').read()
while True:
    GTIN=(input("please enter GTIN for product"))
    if GTIN.isnumeric() and len(GTIN)==8 and GTIN in file_contents:
        total2 = 0
        i = 0
        for n in GTIN:
            v = int(n)
            if i % 2 == 0:
                v = v * 3
            total2 += v
            i += 1
        if total2 % 10 == 0:
            print(GTIN)
        else:
            print("Product Not Found")
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ So there is not way of making lines 5-12 shorter ? \$\endgroup\$ – Jeff1231 Oct 1 '16 at 17:14
  • \$\begingroup\$ @Jeff1231: there is. I have added the code to the answer above. \$\endgroup\$ – Simeon Visser Oct 1 '16 at 17:19
  • \$\begingroup\$ @Jeff1231: While in this case making it shorter is preferrable, also keep in mind in the future that shorter code doesn't always mean better code. Something a few lines (or more) longer can be a hundred times faster if the short code is inefficient. And when you work in a team, it is far more preferrable to write something easy to read over what is known as "premature optimization". \$\endgroup\$ – Juha Untinen Oct 3 '16 at 5:10
3
\$\begingroup\$

Oh, God, that's awful. If you are trying to express the idea "the sum of the odd-indexed numbers plus three times the sum of the even-indexed numbers", in Python, that could be written as:

3 * sum(GTIN[0::2]) + sum(GTIN[1::2])

As for the file-handling, I believe that is addressed in other answers.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This looks like shortest readable way to express that thought to me, with a closest alternative of sum(3 * x if index % 2 else x for (index, x) in enumerate(GTIN))and after that maybe some itertools shenanigans \$\endgroup\$ – Daerdemandt Oct 1 '16 at 19:19
2
\$\begingroup\$

@Simeon has covered some good points. What I would add is that looking at your code, your logic seems inverted. You seem to be doing this:

  • Ask User for GTIN
  • Check if GTIN is in file
  • Calculate CheckValue from GTIN and verify it is valid
  • If the CheckValue isn't valid, print that the product isn't found.

This feels wrong. If the GTIN is in the file, then I'd expect it to be have a valid format and hence be found. So, I'd expect the logic to be the other way round:

  • Ask User for GTIN
  • Perform calculation and check if GTIN is valid format
  • Check if it's in the file

The check calculation is fairly lightweight, whereas I would expect the file to contain a large number of items and hence take longer to search.

You may also want to think a bit more about your variable names. Names like total2 are meaningless out of context, try to name the variables for what they represent, not their job in the calculation.

| improve this answer | |
\$\endgroup\$
1
\$\begingroup\$

Profiling comes before optimisation

Before making your program more efficient you have to make sure where those inefficiencies come from.

For example, it might be that your system is has to check lots of entries that were aggregated through several stores - so lots of valid GTINs that must be rejected because they are not in your list. Then, optimizing these lookups would be a top priority.

It might be that your system is feeded with lots of GTIN-looking things, so checking for validity is a top priority.

Lookups

I assume that 'inefficiency' remark from your teacher referred to reading the file in every iteration of the loop. This is inefficient indeed. File should be read outside of the loop, once.

However, having a list and checking if an element is there is not really efficient too. There are other data structures, better suited for that. I'd store those GTINs in a set so that lookups are fast.

Readability

It is important. Sure, some optimizations make code less readable, but usually they are not worth it.

Logic

It seems that invalid entries sometimes (e.g. non-numeric ones) trigger no response at all and sometimes (checksum failed) trigger 'Product Not Found' response. Is that expected behaviour?

Compiling that together I'd get something like this:

def is_valid_GTIN(string):
    if string.is_numeric() and 8 == len(string):
        GTIN = [int(digit) for digit in string]
        if (3 * sum(GTIN[0::2]) + sum(GTIN[1::2])) % 10 == 0:
            return True
    return False

def get_input():
    pass

with open('read_it.txt') as input_file:
    known_GTINs = set(line.strip() for line in input_file)
    while True:
        GTIN = get_input()
        if is_valid_GTIN(GTIN):
            if GTIN in known_GTINs:
                print(GTIN)
            else:
                print('Product not found')
        else:
            print('Invalid GTIN')

With whatever fancy user-interaction I need to get input from user in place of get_input

| improve this answer | |
\$\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.