Printing factors and prime factors of a number

I am a 46 year young newbie trying to learn programming for fun and adventure. I do this in my free time. Some very nice gentlemen on stackoverflow have sent me here. I wish that someone could review my code and find faults with my style and weak areas, so that I might work upon it.

prime_numbers = [2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47, 53, 59, 61, 67, 71, 73, 79, 83, 89, 97, 101, 103, 107, 109, 113, 127, 131, 137, 139, 149, 151, 157, 163, 167, 173, 179, 181, 191, 193, 197, 199, 211, 223, 227, 229, 233, 239, 241, 251, 257, 263, 269, 271, 277, 281, 283, 293, 307, 311, 313, 317, 331, 337, 347, 349, 353, 359, 367, 373, 379, 383, 389, 397, 401, 409, 419, 421, 431, 433, 439, 443, 449, 457, 461, 463, 467, 479, 487, 491, 499, 503, 509, 521, 523, 541, 547, 557, 563, 569, 571, 577, 587, 593, 599, 601, 607, 613, 617, 619, 631, 641, 643, 647, 653, 659, 661, 673, 677, 683, 691, 701, 709, 719, 727, 733, 739, 743, 751, 757, 761, 769, 773, 787, 797, 809, 811, 821, 823, 827, 829, 839, 853, 857, 859, 863, 877, 881, 883, 887, 907, 911, 919, 929, 937, 941, 947, 953, 967, 971, 977, 983, 991, 997 ]
list_of_factors =
list_of_Prime_factors =


def look4factors(number):
global list_of_factors
for index in range (2, 1 + number/2):
if number % index == 0:
list_of_factors.append (index)

#-------------------------------------------------------------------

def find_prime_factors(number):
global list_of_factors
global prime_numbers
global list_of_Prime_factors
quotient = number
while (quotient > 1):
for index in range(0,len(prime_numbers)):
if (quotient % prime_numbers[index] == 0 ):
list_of_Prime_factors.append(prime_numbers[index])
quotient = quotient / prime_numbers[index]

#------------------------------------------------------------------

def print_all_factors(number):
global list_of_factors
print "nnThe Factors for are,".format(number),
for index in range (0, len(list_of_factors)):
print "".format(list_of_factors[index]),
print "",

#-------------------------------------------------------------------

def print_all_prime_factors(number):
global list_of_Prime_factors
find_prime_factors(number)
list_of_Prime_factors.sort()
print "nnThe Prime Factorization for would be = ".format(number),
for index in range (0, len(list_of_Prime_factors)):
print "".format(list_of_Prime_factors[index]),
if (index + 1 == len(list_of_Prime_factors)):
pass
else:
print "X",

#-------------------------------------------------------------------

def main():
str_product = raw_input("Please enter a number to get its factors ")
int_product = int(str_product)
look4factors(int_product)
if len(list_of_factors)== 0:
print "It's a Prime Number."
else:
print_all_factors(int_product)
print_all_prime_factors(int_product)



if __name__ == "__main__":
main()


1 Answer
1

Welcome to programming! I'm new to code reviews (your StackOverflow post brought me here), but hopefully I have enough other experiences that something that follows will be of value to you.

As to your OOP question, since you're already learning it I would stick to Python rather than learning 2+ languages at once. It still has full support for objects and OOP principles, so there isn't a whole lot to lose. If you had to pick between Simula or SmallTalk, I'd recommend SmallTalk in a heartbeat (and I'd recommend temporarily dropping Python so that you're still only learning one language at a time).

I don't generally think critiques should only include the negatives, both for psychological reasons and since it's also important to keep doing the good things. At a high level, all of the following are nice. Keep it up.

You're breaking the code up into modular components. In your case, the components happen to be functions rather than objects, and that's fine. Functional code can express many problems more easily than OOP code.

The #--- comments you're using make the code surprisingly pleasant to read. I haven't seen that before, and I rather like it.

#---

The if __name__ == '__main__' bit is a best practice in Python code, allowing different behavior if your program is called as a script or imported.

if __name__ == '__main__'

Your variable names are descriptive. This IS a big deal and makes everyone's lives easier (including yours). This is an important detail that I've noticed most new programmers leave out.

This looks like a lot in a big wall of text, but I promise there aren't that many things; I'm just trying to make sure that there's enough explanation that the comments are actually helpful to you.

Using global variables is almost universally recognized as a poor programming practice in every major language. The problem is that as the number of connections between different parts of your code increases, the number of code paths increases exponentially. It becomes very hard to reason about the behavior of your program. By having everything linked to even one (or in this case more than one) global variable, it is hard to analyze any component by itself. Even in your current code, if you tried to find the prime factors of several numbers at once you would likely have hard-to-track-down bugs due to the global variables.

One way to approach that option is to pass more arguments in your functions. As one example, instead of having look4factors() and find_prime_factors() both share the global variable list_of_factors, you could have look4factors() return the local variable list_of_factors, and then that could be an argument to find_prime_factors().

look4factors()

find_prime_factors()

list_of_factors

look4factors()

list_of_factors

find_prime_factors()

Capitalization schemes should typically be consistent in your code. The occasional use of Prime instead of prime could cause problems down the road.

Prime

prime

Having hard-coded values should be avoided as much as possible. It's a lot of work for you, and it makes your code less robust too. With that few prime factors, it wouldn't work for numbers much bigger than 1000000 (approximately your biggest prime number squared).

It would be better to generate those prime numbers at run time. You could do that with trial division (for each new number, check to see if it's prime by checking if all the primes you've found up to the square root of that number so far are factors), or with more complicated algorithms like the Sieve of Eratosthenes. The key point is that by doing so you can handle any size of input instead of just small inputs.

Since you're using Python 2, the use of range() is discouraged when you're just iterating over those values. The range() function creates an object storing every integer you'll iterate over, whereas xrange() simply keeps yielding the new values. It saves a lot of RAM to use xrange(). This is one of the things that broke between Python 2 and Python 3. In Python 3, xrange() doesn't exist by default, and range() behaves like the old xrange() used to.

range()

range()

xrange()

xrange()

xrange()

range()

xrange()

When you have a list of things (like list_of_Prime_factors) and need to do something special for just the beginning or just the end, rather than having an if statement inside your loop, it's better to have the special case either before or after the loop and have the loop only do the main operation. This matters because

list_of_Prime_factors

if

if

if

Python has a concept of iterables that you might find helpful in simplifying your code. You can do other things than iterables, but one idea is that instead of writing

for i in (0, len(thing)):
# stuff with thing[i]


You can write something like

for item in thing:
# stuff with item


Most libraries, services, and security updates have been implemented in or completely switched to Python 3. If it were only for that, I'd recommend switching. There are some syntactical things that make your life easier too though, even in this code example. In Python 3.6+ you have so-called f-strings which allow arbitrary expressions inside them. Instead of ''.format(x) you can just write x, and the code runs faster too.

''.format(x)

x

Note that many common things that you can do in a loop have already been implemented in the language. For example, it's possible to replace all of

for index in range (0, len(list_of_Prime_factors)):
print "".format(list_of_Prime_factors[index]),
if (index + 1 == len(list_of_Prime_factors)):
pass
else:
print "X",


With the following line of code

print ' X '.join(map(str, list_of_Prime_factors))


Or if you'd rather learn about Python's list comprehensions, the following is roughly the same thing (and still works in Python 3 after modifying print to be a function, where the example I just gave would need a little tweaking).

print

print ' X '.join([str(p) for p in list_of_Prime_factors])


By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.