Is this factorial function from a C tutorial wrong?

Question

I have a little programming experience in Python, and for the sake of curiosity I started to learn C, at least the very basics. In a tutorial, I found an example function for calculating a factorial, which is this:

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

I also added these lines in order to see the output:

#include <stdio.h>

int main()
{
    int val;
    val = factorial(5);
    printf("%d\n",val);
    return 0;
}

Then I compiled the code with gcc factor.c -o factor.out, run and... ooops, the result is -1899959296, clearly there's something wrong.

After a couple of tries, using printf() to print both i and x, I figured out what went wrong (I think): since the condition of the for loop checks if the counter is less than x, and at each step x gets bigger and bigger, i is always less than x, so the loop keeps going on, presumably stopping when the value of x is too big (which should be related to int, I'm not very familiar yet with the various data types).

So, to "solve" the problem, I rewrote the function like this:

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

I compiled the program, run, and the value printed is 120, which is correct.

I searched a little bit for examples of factorial functions in C, and found many solutions much better than mine, taking into account negative numbers and formats like long and whatnot, but all of them, in a way or another, seem to rely on two "main" variables, much like my solution.

So, the final question: is the example proposed wrong, or am I missing something? This really bothers me, because if such a simple example is blatantly wrong, I should question the credibility of the rest.

Answer 1

Yes, indeed. The original factorial function is wrong for exactly the reason you've identified. You can't use x as both the accumulator and loop limit.

In your version, one improvement I'd suggest is to get rid of counter. Since you're no longer modifying x, you don't need to save off a copy of it.

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

You could do this without additional variables if you reverse the loop.

int factorial(int x)
{
    int i;
    for (i = x - 1; i > 1; i--)
    {
        x *= i;
    }
    return x;
}

I wouldn't write it this way, though. Modifying an input parameter is poor style.