Your code is perfectly fine as the solution to a programming exercise.
If you need to calculate factorials as part of a real-world production system, use math.factorial()
so you don’t reinvent the wheel. (This is a lesson you learn as you get experience as a software engineer.)
If you are interested in writing “more maintainable” code, you should have the function raise ValueError
instead of return a string. This way, the function always returns an int. You also want to add type hints to your function.
If you want the code to more maintainable by shortening it, you could get rid of the n == 0
case:
def factorial(n):
if n < 0:
raise ValueError('arg must be a positive int')
else:
result = 1
for i in range(1, n + 1):
result *= i
return result
The for
loop does a needless * 1
multiplication if n
is 1
but it makes the code simpler. You are exchanging performance for an increase in code readability, which is a fair thing to do sometimes. (The entire value of code is not just how few nanoseconds it takes to run. This is something you learn as you get experience as a software engineer.)
Since the exception sends the execution out of the function, you could make that else
an if
, which gets rid of one level of indentation for the rest of the code:
def factorial(n):
if n < 0:
raise ValueError('arg must be a positive int')
result = 1
for i in range(1, n + 1):
result *= i
return result
You can also trade memory for performance. For example, if you precalculate a bunch of factorials, you can re-use those calculation results or use the last one as a starting point if you need to calculate a larger factorial than what you have cached:
CACHE = {0: 1, 1: 1, 2: 2, 3: 6, 4: 24, 5: 120, 6: 720, 7: 5040, 8: 40320, 9: 362880, 10: 3628800}
CACHE_MAX_N = max(CACHE.keys())
FACT_OF_MAX_N = CACHE[CACHE_MAX_N]
def factorial(n):
if n < 0:
raise ValueError('arg must be a positive int')
if n in CACHE:
return CACHE[n]
result = FACT_OF_MAX_N
for i in range(CACHE_MAX_N + 1, n + 1):
result *= i
return result
How many values you want to cache is a judgement call on what’s more important for your application: fast speed or low memory usage. But keep in mind that factorials grow so large so fast that even 2000! can’t be converted to string to display it in Python with increasing the default limit:
>>> math.factorial(2000)
Traceback (most recent call last):
File "<python-input-3>", line 1, in <module>
math.factorial(1600)
~~~~~~~~~~~~~~^^^^^^
ValueError: Exceeds the limit (4300 digits) for integer string conversion; use sys.set_int_max_str_digits() to increase the limit
All of these issues come up and the answer is always “it depends”. It depends on what size factorials you’ll be calculating (like, 40! or 4000000000000!) and whether you need them fast or memory is limited. Or maybe the schedule doesn’t give you time to come up with some optimum solution so you should just stick to the already-existing math.factorial()
(in my experience, this is the most common case and best solution.)
But like I said, your code is perfectly fine too.