Unable to get a factorial function to work in C

Question

I cannot get the following code to work.

#include <stdio.h>

// I am not sure whethere I should void here or not.
int main() {
    // when the first bug is solved, I put here arg[0]. It should be
    // similar command line parameter as args[0] in Java.
    int a=3;                  
    int b; 
    b = factorial(a);

    // bug seems to be here, since the %1i seems to work only in fprintf
    printf("%1i", b);
    return 0;      
}  

int factorial(int x) {
    int i; 
    for(i=1; i<x; i++) 
        x *= i; 
    return x; 
}

How can you get the code to work?

Answer 1

What error message do you get?

First off, declare your function factorial before main. Also, pay attention to correct indentation. Your declaration of function main is correct, by the way.

Answer 2

You're modifying your loop terminating variable (x) inside the loop. Currently your code blows up after a few iterations, when x overflows the range of a 32 bit integer and then becomes negative and very large, hence terminating the loop.

It should be:

int factorial(int n) {
    int i, x = 1;
    for (i = 2; i <= n; ++i) {
        x *= i;
    }
    return x;
}

Better yet, you should use long instead of int for the variable x and the return value, because n! gets very large very quickly.

Answer 3

AInitak gave the right answer, but I want to add that one way you can find the bug in your code is to print out the values of i and x in the factorial loop.

int factorial(int x) {
    int i;
    for(i=1; i<x; i++)
    {
        x *= i;
        printf("%d, %d\n", i, x);
    }
    return x;
}

This gives you the output

1, 3
2, 6
3, 18
4, 72
5, 360
6, 2160
7, 15120
8, 120960
9, 1088640
10, 10886400
11, 119750400
12, 1437004800
13, 1501193216
14, -458131456
-458131456

This makes it easier to see what's going wrong. The loop doesn't stop where you expect it to for the reasons AInitak explained.

Answer 4

I would suggest to use also double or unsigned long for factorial computation in order to be able to compute the greater value of the factorial function.

double fact( double n)
{
   if ( n == 1)
        return 1;
   return n*(fact(n-1));
}

Answer 5

It's bad style in C to leave out void when defining or declaring a function. So put it in

int main(void)

While it doesn't change anything about the number of parameters the function has (the function has zero parameters without that void either), it will declare the function as one that accepts only zero arguments, while it won't tell anything about the amount and types of accepted arguments when you omit the void. However, both versions with and without void are correct.

Read this answer about that matter too.