HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Finding factorials

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
factorialsfindingstackoverflow

Problem

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.

Solution


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

Code Snippets

print('!' + str(number) + ' = ' + str(factorial))
"""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))

Context

StackExchange Code Review Q#127839, answer score: 5

Revisions (0)

No revisions yet.