0
\$\begingroup\$

What I need to do is to return the prime numbers that are dividers of a some numbers in a list, so if

I = [12, 15] # result = [[2, 12], [3, 27], [5, 15]] 

and this is the code that I used for that:

def sum_for_list(lst):
    r = []
    for num in lst:
        for i in range(2, abs(num)+1):
            if num%i == 0 and i%2 != 0 and all(i%x != 0 for x in r) and i not in r:
                r.append(i)
            elif i == 2 and num%i == 0 and 2 not in r:
                r.append(i)


    newList = []
    for r in r:
        z = 0
        for y in lst:
            if y%r == 0:
                z += y
        newList.append([r,z])
    
    return sorted(newList) 

BTW, if you know about good resources to study efficient algorithms, I would really appreciate that too :)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ May 18, 2021 at 13:48

1 Answer 1

3
\$\begingroup\$

Here are some suggestions for improving readability of your code. The more readable / easily understandable your code is, the easier it is to properly review regarding performance.


Naming

This is probably the most important point regarding your code. More or less none of the variables have meaningful names. This makes it that much harder to understand your algorithm.

Single letter variable names (r, z, y) are only a good idea when following well-known conventions like iterating over an index (for i in range(...)). The same goes for lst and newList, these names say nothing about content and function of the variables. Without fully understanding your algorithm I would presume some names like divisor, numbers, candidate_primes, results, etc. might be useful.

Variables should be named in snake_case instead of camelCase. PEP 8

for r in r:: Using the same variable name for different variables (in this case: the single elements of an iterable and the iterable itself) is confusing to the reader and might lead to some unexpected behaviour in some cases.


Conditions

Line 5: After checking all(i%x != 0 for x in r), you don't need to additionally check if i not in r. Since i % i == 0, the first condition would already fail if i in r.


Comments & docstrings

You should include comments and docstrings in your code to explain at least the key functionality / algorithmic approach.


You can remove the entire elif case from this for loop:

for num in lst:
    for i in range(2, abs(num)+1):
        if num%i == 0 and i%2 != 0 and all(i%x != 0 for x in r) and i not in r:
            r.append(i)
        elif i == 2 and num%i == 0 and 2 not in r:
            r.append(i)

like this:

for num in lst:
    if num % 2 == 0 and 2 not in r:
        r.append(2)

    for i in range(3, abs(num) + 1):
        if num % i == 0 and i % 2 != 0 and all(i % x != 0 for x in r) and i not in r:
            r.append(i)

This will save you some unnecessary checks for each i in range(3, abs(num) + 1) for each num.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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