3
\$\begingroup\$

I just started teaching myself to code a few days ago using the Euler problems. I finished a variant of problem 3: print the prime factors of a number input by the user. I just want feedback and code cleaning from someone who knows better than I do to help me progress.

import math

#What is the largest prime factor of the number 600851475143?

n=raw_input('Enter the integer you wish to find the prime factors of: ')

n = int(float(n))
L=[]

def too_small():
        """to be called for int < 4"""
        L.append(n)
        if n>0:
                L.append(1)
        return L

if n < 4: #alg only works for int > 4
        L = too_small()
        print L
        exit()

if n == 4: #because I'm lazy
        L.append(2)
        L.append(2)
        print L
        exit()

def primefactor():
        """prints list of all prime factors for int > 4"""
        test=True
        b=n
        while(test):
                i=2
                while((b%i!=0)&(i<(b/2))): #find next prime
                        i=i+1
                if i>=(b/2): #number is prime
                        L.append(b)
                        L.append(1)
                        test=False
                else: #number is not prime
                        a=(b/i)
                        L.append(i)
                        b=a
        return L

L = primefactor()
print L

exit()
\$\endgroup\$
3
\$\begingroup\$

Consistency

Why is there sometimes a 1 at the end of the results? Should it even be there, considering that 1 is not a prime number? If you do include it in the results, why is it at the end instead of at the beginning of the list?

Enter the integer you wish to find the prime factors of: 60
[2, 2, 3, 5, 1]

Enter the integer you wish to find the prime factors of: 5
[5, 1]

Enter the integer you wish to find the prime factors of: 4
[2, 2]

Lint

PEP 8, the official style guide, specifies 4 spaces per level of indentation. This is a strong convention for Python, where whitespace is significant.

Also, by PEP 8, primefactor() would be better named prime_factors().

It makes no sense to convert n to a float, then convert it again to an int.

import math is pointless; you never use anything in the math module.

Some of your comments are unclear. For example:

if i>=(b/2): #number is prime

… would be better written as # b is prime.

Since this is all integer arithmetic, you can prepare your code for use with Python 3 by using the // operator rather than /.

Counting loops in Python are better written using some kind of for loop in conjunction with an iterator. Here, I recommend itertools.count(2).

Generality and organization

primefactor() would be better as a function that accepts a parameter, rather than using the global variable n. It should definitely return a list that it created, rather than modifying a global L as a side-effect.

Why doesn't primefactor() also work for n ≤ 4? If you organized the code properly, the caller wouldn't be responsible for handling special cases (which aren't even necessary in the first place). There would be no need to call exit() — which you should almost never need in a properly structured program.

Flag variables, especially poorly named ones like test, are rarely desirable or necessary. Here, you can easily eliminate the test variable by changing test=False to return L.

First rewrite

Incorporating the remarks above, and more…

from itertools import count

def prime_factors(n):
    factors = []
    for i in count(2):
        while n % i == 0:
            factors.append(i)
            n //= i
        if 2 * i > n:
            return factors

n = int(raw_input('Enter the integer you wish to find the prime factors of: '))
print(prime_factors(n))

Yielding results

You can stream the results back to the caller by yielding prime factors as you find them, rather than building the entire list.

As an optimization, you can avoid considering even factors other than 2.

from itertools import chain, count

def prime_factors(n):
    for i in chain([2], count(3, 2)): # Try 2, 3, 5, 7, 9, 11, 13, 15...
        while n % i == 0:
            yield i
            n //= i
        if 2 * i > n:
            return 

n = int(raw_input('Enter the integer you wish to find the prime factors of: '))
print(', '.join(prime_factors(n)))
\$\endgroup\$
  • 1
    \$\begingroup\$ Very good answer as always, the Generality and organization section is especially golden advice even more so to a beginner. Using yield is surely an improvement but given the basic-ness of this question I preferred not to mention it as it is a rather advanced topic. I think it is good that you reinforced some of my comments like It makes no sense to convert n to a float, then convert it again to an int. or If you organized the code properly, the caller wouldn't be responsible for handling special cases or import math is pointless it is a double check to my answer. And good job on... \$\endgroup\$ – Caridorc May 24 '16 at 18:46
  • 1
    \$\begingroup\$ addressing the algorithm of the main factorizing function by seriously simplifying it as I did not touch on it at all. Good TeamWork on this one! \$\endgroup\$ – Caridorc May 24 '16 at 18:47
  • 2
    \$\begingroup\$ Really, thank you both. This is everything I was hoping for. I can't even imagine how much learning time this feedback saved me. In response to your comment: please don't shy away from the more advanced topics, no matter how basic the original question might be. If I'm not ready for it, I can always mark it down to incorporate later on. I always appreciate a new topic to dive into. \$\endgroup\$ – JustAnotherBot May 24 '16 at 19:00
  • \$\begingroup\$ Your code doesn't calculate all the prime factors for many numbers. \$\endgroup\$ – Chani Mar 17 '18 at 15:44
  • \$\begingroup\$ @Chani Can you provide an example where it is wrong? \$\endgroup\$ – 200_success Mar 17 '18 at 16:44
2
\$\begingroup\$

Functions should have inputs and outputs

def too_small():

This function acts on two global variables n and L.

You need to prepare such variables with exactly such names or the function will fail.

This function also modifies the global variable L that is a huge no-no unless clearly expected and/or (better and) clearly documented.

The name is also not informative as it makes no mention to the fact that it builds a list of prime factors.

Unnecessary math import

math is imported but not used, remove unused parts of the code such as this import statement to reduce clutter.

Unnecessary / Undocumentedfloat conversion

You explicitly state that an integer must be inputted, there is no reason to convert it to a float (such conversion is immediately overridden by the conversion to int).

If you use float to allow inputs with a decimal point followed by zeros (ex 412.0) you should explicitly state so in a comment as seeing int(float is very surprising and you should not leave the reader wondering.

Lack of module-level doc-string

You should at least in a phrase describe what your code does, stating so in the question body is good, stating it in the code itself is better.

Just add

"""
This script prints the prime factors of a number input by the user.
"""

to the top of your file and remove:

#What is the largest prime factor of the number 600851475143?

as such number is nowhere else found in the code.

0 corner case

You return [0] for 0 input but 0 is evenly divided by all the prime numbers, so your answer is wrong. I suggest raising an exception (or issuing a special message) in case 0 is the input.

Unnecessary fragmentation

You would really simplify by writing exactly one function to factorize any number and calling just that.

Be accurate in documentation

def primefactor():
        """prints list of all prime factors for int > 4"""

Wrong documentation is worse than no documentation at all.

print means output to the screen. This function produces no output.

You meant return that means "giving a value back to the program from a function"

Segregate function definition and main part

Function definitions are inter-mangled to statements that "do stuff" in your program.

A universal convention to achieve a better organization is defining all your functions on the top and than putting the main code under an if __name__ == "__main__" statement at the bottom.

This way you also make it possible to import your program and use it as a module without the user interactions running.

Remove exit call(s)

Quoting from StackOverflow

However, like quit, exit is considered bad to use in production code and should be reserved for use in the interpreter. This is because it too relies on the site module.

In other words exit was made to help new programmers in using the interactive interpreter and should not be used in a stand-alone program.

\$\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.