5

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);
}
Steve Summit
  • 45,437
  • 7
  • 70
  • 103
ash54321
  • 133
  • 1
  • 9
  • It is giving `6` for 4! and `24` for 5! and `120` for 6! Do you see a pattern? – Weather Vane Jun 17 '21 at 18:39
  • 3
    `warning: operation on ‘number’ may be undefined [-Wsequence-point]` – wildplasser Jun 17 '21 at 18:40
  • yes but i want know why my code is wrong where did i do something wrong if i replace --number with the number-1 i get the right answer though – ash54321 Jun 17 '21 at 18:41
  • In `number*factorial(--number)` what will be the value used for the *first* `number`? The `number` is decremented at *some time* between the previous sequence point and `--number`. But nobody knows exactly when. – Weather Vane Jun 17 '21 at 18:44
  • how come `number` gets replaced by `--number` even before the execution of `--number` – ash54321 Jun 17 '21 at 18:50
  • Because the pre-decrement happens *some time* after the previous sequence point. And the function is called before the multiplication is done. – Weather Vane Jun 17 '21 at 18:54
  • One more tip. `++` and `--` can be fantastically useful operators. But please don't use them when you don't need them. Do you remember why you wrote `factorial(--number)` in the first place? Do you see how `factorial(number-1)` does exactly what you need? You do need to subtract 1 from `number`'s value before passing it to `factorial()` for the recursive call, but you don't need to store anything back into `number`. Remember, `--` doesn't just mean "subtract 1". It means, "subtract 1 and store back into". – Steve Summit Jun 17 '21 at 19:58

2 Answers2

10

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?

Steve Summit
  • 45,437
  • 7
  • 70
  • 103
  • but it works if we used `number - 1 ` instead of `--number` – IcanCode Jun 17 '21 at 18:49
  • 1
    @IcanCode `number - 1` does not change the value of `number` but `--number` does. – Weather Vane Jun 17 '21 at 18:58
  • i am using vscode but i can t see any warnings or _undefined behavior_ alert – ash54321 Jun 17 '21 at 19:02
  • Maybe it just can't detect this undefined behavior in particular or maybe it expects you're doing it on purpose because you know what your compiler is going to do and you're not going to use a different compiler – altermetax Jun 17 '21 at 19:04
  • 2
    @ash54321 Unfortunately, explicit warnings about undefined behavior are quite rare. You have to know to avoid it by yourself. The compiler won't aways warn you -- in fact in some cases it *can't* warn you. – Steve Summit Jun 17 '21 at 19:08
  • Isn't this technically unspecified behavior, and still valid from the compilers view point. Number is only modified once in the expression, but the compiler is allowed to do it in any order it wants. – jo-art Jun 17 '21 at 21:53
  • 1
    @jo-art It is undefined. See citations at the [linked question](https://stackoverflow.com/questions/949433/why-are-these-constructs-using-pre-and-post-increment-undefined-behavior). – Steve Summit Jun 17 '21 at 22:03
  • @ash54321 Undefined behavior is kind of like jaywalking on a big highway. You might get away with it, especially in the middle of the night when there's no traffic. A policeman might give you a warning. A policeman might give you a citation. Or you might get run over by a semi. – Steve Summit Jun 18 '21 at 17:30
0

How to find the factorial of a number;

function factorial(n) {
    if(n == 0 || n == 1 ) {
        return 1;
    }else {
    return  n * factorial(n-1);  
    }
    //return newnum;
}
console.log(factorial(3))
user438383
  • 5,716
  • 8
  • 28
  • 43
S Gabale
  • 1
  • 3