Finding factorials

Question

How can this script be optimized?

def processor(n):
 """Finding the factorial of a given number. """
 if n == 0 or n == 1:
 return 1
 product = 1
 for i in range(1, n + 1):
 product *= i
 return str(n) + '! = ' + str(product)
def guardian():
 """Asking for a number and guarding the script from raising an error. """
 number = input('Number: ')
 if not number.isdecimal():
 print('Please enter a positive integer number! \n')
 guardian()
 else: 
 print(processor(int(number)))
guardian()

Notes:

• I'm new to factorials and have just understood the basic idea, so I'm sorry if I'm not precise.
• I'm a hobbyist and a beginner.

Answer 1

• It may be much cleaner to put the isdecimal() check in a loop, rather than involving recursion (a function calling itself). You can then put the call to print() after the call to guardian() at the very bottom.
• As for processor(), it's already calculating the factorial, so don't have it convert output as well. Just return the number and let another function handle the output.

• The methods could have more specific names to make their intents clearer at a glance. This could also eliminate the need for the initial comments (unless they're specifically for having documentation, for which they should be right before the method name).

  For instance, guardian() can be renamed to acquire_number() and processor() could be renamed to calculate_factorial().

• You could consider having more variables at the bottom to avoid cramming so many function calls inside each other. This will help make it a bit more readable.

• The final output could look a bit nicer:

  print('!' + str(number) + ' = ' + str(factorial))
  

Solution with all changes:

"""Finding the factorial of a given number. """
def calculate_factorial(n):
 if n == 0 or n == 1:
 return 1
 product = 1
 for i in range(1, n + 1):
 product *= i
 return product
"""Asking for a number and guarding the script from raising an error. """
def acquire_number():
 number = input('Number: ')
 while not number.isdecimal():
 print('Please enter a positive integer number! \n')
 number = input('Number: ')
 return number
number = acquire_number()
factorial = calculate_factorial(number)
print('!' + str(number) + ' = ' + str(factorial))

Answer 2

The factorial function looks more complicated than it should be. You are starting by multiplying by 1, which is not all that useful. You can also use the fact that the range where the start is beyond the end is empty to your advantage. Last but not least, you are returning two different things. One is some formatted string, the other being an integer.

By correcting this, you would get something like this:

def processor(n):
 """Finding the factorial of a given number. """
 if n == 0:
 product = 1
 else:
 product = n
 for i in range(2, n):
 product *= i
 return str(n) + '! = ' + str(product)

• zondo: I think it would be better to return a number rather than the formatted string. That makes the function a lot more reusable.

Answer 3

Iterative (like you were doing):

def factorial(n):
 out = 1
 for i in range(1, n + 1):
 out *= i
 return out

Recursive (simpler than iteration, but less efficient):

def factorial(n): 
 if n == 0:
 return 1
 return factorial(n-1) * n

Reduce function (one-liner, not sure how it compares in terms of efficiency):

def factorial(n): 
 return reduce(range(1, i+1), lambda a,b: a*b)

Built-in factorial function (much simpler, should be better optimized):

from math import factorial

Wraper to do I/O:

if __name__ == "__main__":
 number = ""
 while not number.isdigit():
 number = input("Number: ")
 if not number.isdigit():
 print('Please enter a positive integer number! \n')
 print(number, '! = ', str(factorial(int(number))), sep="")

• mdfst13: Hi. Welcome to Code Review! We usually prefer our answers to be more definite. While a Socratic question may be well taken, answers aren't generally the place for unanswered questions. This would be a better answer if it explained why someone should use recursion rather than asking why not. Note that iterative solutions are often faster and lower overhead than recursive solutions. And in this particular case, your function returns a number while the original function usually returns a string.
• Solomon Ucko: @mdfst13 Sorry I didn't notice it returned a string, and for the lack of explanation as to why it was better, and also, it was around 10pm for me when I answered this. Should I delete it?
• Peilonrayz: @SolomonUcko If you think you can edit it to make it an answer we accept then edit it. If you'd rather delete it, then delete it.
• Solomon Ucko: @mdfst13, can I just leave it incase people come across it and think it's useful?
• Karthik Kumar Viswanathan: 1. Use isdigit() not isdecimal(). 2. Use raw_input() if you want to check a string type. input() returns a numeric type if the entered input is a number. If you do enter a string, Python would eval it as a symbol. 3. Use: print(number, '! = ', factorial(int(number))) to avoid concatenation and explicit type conversion to string.

Answer 4

I assume by optimizations you mean speed? I will use the answer you have flagged to rewrite.

def processor(n):
 """Finding the factorial of a given number. """
 if n == 0:
 product = 1
 else:
 product = n
 for i in xrange(2, n): '''xrange is an iterator and will calculate inplace rather than generating a list
 assuming you are using pre python 2.7, will not come into play for 
 smaller n values however with an n of 1e6 or > the difference will be 
 monumental'''
 product *= i
 return ' '.join((str(n), '!=', str(product))) #I also recommend join as it calculates slightly faster than concatenating

• Solomon Ucko: Did you notice the python-3.x tag on the question?