Printing factors and prime factors of a number

Question

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 "\n\nThe 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 "\n\nThe 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()

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).

Positives

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.

• 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.

Needs Improvement

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().

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

• 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.

• 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

  • if statements and other branches are more expensive for modern CPU's to deal with than other kinds of operations. Arguably that doesn't matter as much in Python since everything is so abstracted and since some of those branches are inlined by the bytecode compilation step, but maybe it's a tidbit you'll find useful one day.
  • Nested code is harder to reason about and debug than flat code.
  • Whether if statements are comparatively more expensive or not, you're still doing a lot of them that you don't need to.

• 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

Extras

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.

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 ' X '.join([str(p) for p in list_of_Prime_factors])