Reputation: 152
i have written a small function which calculates factorial for a number in C as follows:
int factNnumbers(int n)
{
if(n == 1)
return 1;
else
return (n*factNnumbers(--n));
}
I call the function shown above as:
factNnumbers(takeInputN());
where the function to take the input (takeInputN
) is defined as:
int takeInputN()
{
int n;
printf("\n\nHow many numbers ?? \n ");
scanf("%d", &n);
return n;
}
If I change one line in my factorial code as shown below , my program works perfectly. Otherwise with the above code it prints the factorial of the number input -1
(example if number input is 5
, it will print the factorial of 4
). Why is this happening?.
int factNnumbers(int n)
{
if(n != 1)
return (n * factNnumbers(--n));
}
Upvotes: 3
Views: 754
Reputation: 7627
I think it’s better to use the tgamma function from math.h for double precision calculations involving factorials (such as Poisson distribution etc.): tgamma(n+1) will give you factorial(n).
Factorial function is increasing very fast, and this will work also for values too big to fit an integer type.
Upvotes: 0
Reputation: 11
the rule of decrement: X = Y-- first passes the value of I to X and decrements after X = --I first pass and decrements the value decremented X
for your case, you decrement the value of parameter passed as argument to the function factNnumbers. Therefore remedy this error, I invite you to put (n-1) instead of (--n).
Upvotes: 0
Reputation: 171197
The problem is that you're both reading and modifying n
in the same expression:
n * factNumbers(--n)
The evaluation of the n
argument to *
and of the --n
subexpression are unsequenced, which gives your code Undefined Behaviour.
The easiest solution (and also, IMO, more expressive), is to say n - 1
where you mean it:
n * factNumbers(n - 1)
Your "improved" code in the bottom of the question is actually even more wrong. There, you have a control path which will return an unspecified value: a clear no-no.
Note: This answer was written while the question still had a C++ tag, and uses C++ terminology. The end effect in C is the same, but the terminology might be different.
Upvotes: 6
Reputation: 19627
There are two causes of undefined behavior in your code:
Whether n
or --n
in n * factNnumbers(--n)
will be evaluated first is unspecified.
See this. You want just n * factNnumbers(n - 1)
, why decrement? You're not using decremented n
afterwards (at least you didn't want to).
You're not returning a value on all control paths, what's going to be returned on n == 1
? An indeterminate value that will mess up the whole result.
Upvotes: 2
Reputation: 6462
When n<=1 (or == 1 in your code), the factorial function has to return 1. Futhermore the your --n in you code is false and sould be n-1 as:
function factorial(n)
{
if (n<=1)
return 1;
else
return n * factorial(n-1);
}
Upvotes: -1
Reputation: 1579
You are invoking undefined behaviour, that it works in one version is just an accident:
return (n*factNnumbers(--n));
Do you use n
first and then decrement it or the other way around? I don't know and neither does the compiler, it's free to do either of them or format your hard drive. Just use n * f(n - 1)
.
Also, your "working" version does not return for the n==1
case, which is illegal.
Upvotes: 6