-1
\$\begingroup\$

Print the number of common factors of a and b.

input > 10, 15

Output > 2

The common factors of 10, 15 are 1 and 5

My code

def print_factors(x,y):
    l = []
    for i in range(1, x + 1):
        if x % i == 0:
            l.append(i)
    m = []
    for i in range(1, y + 1):
        if y % i == 0:
            m.append(i)
    print (list(set(l).intersection(m)))
    #len(print (list(set(l).intersection(m))))


num1 = int(input("Enter a number: "))
num2 = int(input("Enter a number: "))


print_factors(num1,num2)

Is there any better way to optimize, like list comprehension. or using zip module

\$\endgroup\$

closed as off-topic by πάντα ῥεῖ, 200_success, dfhwze, VisualMelon, Sᴀᴍ Onᴇᴌᴀ Aug 17 at 5:37

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Code not implemented or not working as intended: Code Review is a community where programmers peer-review your working code to address issues such as security, maintainability, performance, and scalability. We require that the code be working correctly, to the best of the author's knowledge, before proceeding with a review." – πάντα ῥεῖ, 200_success, dfhwze, VisualMelon, Sᴀᴍ Onᴇᴌᴀ
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 3
    \$\begingroup\$ What is the intended output? The number 2 or the list [1, 5]? \$\endgroup\$ – Martin R Aug 16 at 13:12
  • \$\begingroup\$ @sim if you accept some answer, I guess you might want to upvote that answer as well using the ^ sign. \$\endgroup\$ – Emad Boctor Aug 16 at 18:41
  • \$\begingroup\$ I am voting to close as off-topic because the code and specification do not agree, and so the code cannot be considered working. \$\endgroup\$ – VisualMelon Aug 16 at 22:27
0
\$\begingroup\$

Indentation & style

One of the most distinctive features of Python is its use of indentation to mark blocks of code. Your code is not properly indented (each level of indentation should be equivalent to 4 spaces not like this:

def print_factors(x,y):
 l = []

like this:

def print_factors(x,y):
    l = []

Check pep8 https://www.python.org/dev/peps/pep-0008/ the official Python style guide, you'll find it helpful when learning the basics of Python code style and naming conventions.

Return

a function should not print anything, it should return

print (list(set(l).intersection(m)))
#len(print (list(set(l).intersection(m))))

and print is a function in Python 3.x, so it's very obvious it has no length len(the commented line).

len

len() is used to get the length of sequences:

my_list = [1, 2, 3, 4]
print(len(my_list))

output: 4

Docstrings

Python documentation strings (or docstrings) provide a convenient way of associating documentation with Python modules, functions, classes, and methods. An object's docsting is defined by including a string constant as the first statement in the object's definition. it is generally a good practice to use docstrings in your functions explaining what the function does that are usually enclosed within triple quotes:

def print_factors(number1, number2):
    """Return factors of number1 and number2."""
    # do things
    # return something

Descriptive variable names

l = []
for i in range(1, x + 1):
    if x % i == 0:
        l.append(i)
m = []
for i in range(1, y + 1):
    if y % i == 0:
        m.append(i)
print (list(set(l).intersection(m)))

what is l, what is i what is x ... names should be descriptive to improve the readability of your code (whether people that are reading your code and trying to understand or you reading your own code in the future) and here's an example:

factors_1 = []
factors_2 = []
for divisor in range(1, number1):
    if number1 % divisor == 0:
        factors1.append(divisor)
for divisor in range(1, number2):
if number2 % divisor == 0:
    factors2.append(divisor)

The code

  1. Your function is not even doing what it should be doing (returning the number of common factors not the factors themselves.
  2. You don't have to iterate over the whole range of the number, you iterate up to the square root of the number is sufficient to find all factors.

This is how the code might look like:

def get_factors(number):
    """Generate factors of number."""
    yield 1
    for divisor in range(2, int(number ** 0.5) + 1):
        if number % divisor == 0:
            if number // divisor != divisor:
                yield divisor
                yield number // divisor
            if number // divisor == divisor:
                yield divisor


if __name__ == '__main__':
    number1 = int(input('Enter first number: '))
    number2 = int(input('Enter second number: '))
    number_1_factors = set(get_factors(number1))
    print(f'Factors of {number1}: {number_1_factors}')
    number_2_factors = set(get_factors(number2))
    print(f'Factors of {number2}: {number_2_factors}')
    common_factors = number_1_factors & number_2_factors
    print(f'Number of common factors of {number1} and {number2}: {len(common_factors)} factors.')
\$\endgroup\$

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