0

i have a problem when i try to find if a number is factorial this is what I've done so far:

i = 1
count = 0
while n>1:
    if n % i == 0:
        n /= i
    else:
        break
    i+=1
    if n<=1:
        count += 1
return count

but it returns 1 and I don't know how to fix

mAndAle
  • 17
  • 3
  • Do you want to calculate factorial of a number? – Pouya Esmaeili Dec 04 '20 at 09:45
  • Does this answer your question? [recursive factorial function](https://stackoverflow.com/questions/4502429/recursive-factorial-function) – Pouya Esmaeili Dec 04 '20 at 09:46
  • 1
    If it returns 1, the number you are checking is a result of a factorial. To get the count to be bigger, you need to run the first part of the code in a loop on multiple numbers. Right now you are checking only a single number, returning 0 or 1. – Shamis Dec 04 '20 at 09:51
  • @shamis can you make an example please? – mAndAle Dec 04 '20 at 10:50

2 Answers2

1

As written, the value of count in your example code indicates whether or not the number n is a factorial number, for a single n.

If it is, then at some point n <= 1, so count will be incremented, and the value of count returned by the function will be 1. If it is not, then at some point before n <= 1, n % i == 0 will be False, the loop will break, and the returned value of count will be 0.

Note, though, that there is an edge case not covered by your example code as is. Namely, 1 is a factorial number (1! = 1). But if n is 1, the condition on the while loop is never True, so the function immediately returns 0. So, you need to change the condition in the while loop.

E.g.

def isfactorialnumber(n):
    i = 1
    count = 0
    while n >= 1:
        if n % i == 0:
            n /= i
        else:
            break
        i += 1
        if n <= 1:
            count += 1
    return count

I have 2 further comments about this code.

First, in the context of this new function, the variable name count is misleading. What is it counting? Nothing, really. It can only take 2 values, 1 or 0. It would be better then, to use a boolean variable, and call it result or similar. Even better, get rid of the variable and refactor your code as:

def isfactorialnumber(n):
    i = 1
    while n >= 1:
        if n % i == 0:
            n /= i
        else:
            break
        i += 1
        if n <= 1:
            return True
    return False

Which is equivalent but a little better.

Second, a more readable, natural way to solve the problem would be to work from bottom-up. That is, solve the problem by generating the factorial numbers, m that are less than or equal to n, then checking if the last m is equal to n. This leads to more concise and understandable code IMO.

E.g.

def isfactorialnumber(n):
    m = 1
    i = 1
    while m < n:
        m *= i
        i += 1

    return m == n

Finally, assuming each number in your file, is on its own line:

with open('factorials.txt') as infile:
    print(len([1 for line in infile if isfactorialnumber(int(line))]))

Will print the number of factorial numbers in the file factorials.txt

L.Grozinger
  • 2,280
  • 1
  • 11
  • 22
0
count = 0
for n in listOfNumbers:
    i = 1
    while n>=1:
        if n % i == 0:
            n /= i
        else:
            break
        i+=1
        if n<=1:
            count += 1
return count
Shamis
  • 2,544
  • 10
  • 16