CODEWITHSUNDEEP
cmathrecursion
abhinavm93
abhinavm93

Reputation: 152

C program for calculating factorial

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

Answers (6)

user1978556
user1978556

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

embedded
embedded

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

Angew is no longer proud of SO
Angew is no longer proud of SO

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

LogicStuff
LogicStuff

Reputation: 19627

There are two causes of undefined behavior in your code:

  1. 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).

  2. 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

KeyMaker00
KeyMaker00

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

kamikaze
kamikaze

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

Related Questions