Recursive function for finding factorial of a number

Question

I am getting an output of 24 which is the factorial for 4, but I should be getting the output for 5 factorial which is 120

#include <stdio.h>
int factorial(int number){
    if(number==1){
        return number;
    }
    return number*factorial(--number);
}
int main(){
    int a=factorial(5);
    printf("%d",a);
}

Answer 1

Your program suffers from undefined behavior.

In the first call to factorial(5), where you have

return number * factorial(--number);

you imagine that this is going to compute

       5      * factorial(4);

But that's not guaranteed!
What if the compiler looks at it in a different order?
What it if works on the right-hand side first?
What if it first does the equivalent of:

temporary_result = factorial(--number);

and then does the multiplication:

return number * temporary_result;

If the compiler does it in that order, then temporary_result will be factorial(4), and it'll return 4 times that, which won't be 5!. Basically, if the compiler does it in that order -- and it might! -- then number gets decremented "too soon".

You might not have imagined that the compiler could do things this way.
You might have imagined that the expression would always be "parsed left to right".
But those imaginations are not correct.
(See also this answer for more discussion on order of evaluation.)

I said that the expression causes "undefined behavior", and this expression is a classic example. What makes this expression undefined is that there's a little too much going on inside it.

The problem with the expression

return number * factorial(--number);

is that the variable number is having its value used within it, and that same variable number is also being modified within it. And this pattern is, basically, poison.

Let's label the two spots where number appears, so that we can talk about them very clearly:

return number * factorial(--number);
       /* A */             /* B */

At spot A we take the value of the variable number.
At spot B we modify the value of the variable number.
But the question is, at spot A, do we get the "old" or the "new" value of number?
Do we get it before or after spot B has modified it?

And the answer, as I already said, is: we don't know. There is no rule in C to tell us.

Again, you might have thought there was a rule about left-to-right evaluation, but there isn't. Because there's no rule that says how an expression like this should be parsed, a compiler can do anything it wants. It can parse it the "right" way, or the "wrong" way, or it can do something even more bizarre and unexpected. (And, really, there's no "right" or "wrong" way to parse an undefined expression like this in the first place.)

The solution to this problem is: Don't do that!
Don't write expressions where one variable (like number) is both used and modified.
In this case, as you've already discovered, there's a simple fix:

return number * factorial(number - 1);

Now, we're not actually trying to modify the value of the variable number (as the expression --number did), we're just subtracting 1 from it before passing the smaller value off to the recursive call. So now, we're not breaking the rule, we're not using and modifying number in the same expression. We're just using its value twice, and that's fine.

For more (much more!) on the subject of undefined behavior in expressions like these, see Why are these constructs using pre and post-increment undefined behavior?