Factorial via recursion

[georgewashere]

Hello everyone, I have a few questions about my code.

The exercise question :

Define a recursive function that takes an integer argument and returns the factorial of
that argument. Recall that 3 factorial, written 3!, equals 3 × 2!, and so on, with 0!
defined as 1. In general, if n is greater than zero, n! = n * (n – 1)!. Test your function in a
program that uses a loop to allow the user to enter various values for which the program
reports the factorial

My error is that the factorial function doesn't return a factorial properly. I'm stumped, it will return the product of the first 2 integers but nothing more. Could someone please explain to me where I am failing in logic?

To my knowledge a factorial is : !6 = 6 * 5 * 4 * 3 * 2 * 1

#include <iostream>
using namespace std;
int factorial(int);

int main()
{
    cout << "Enter a number you wish to get the factorial of: ";
    int num = 0;
    while (num >= 0)
    {
        int return_value;
        cout << "\n";
        cin >> num;
        if (num < 0)
            break;
        cout << "Calculating...";
        return_value = factorial(num);
        cout << "\nFactorial of " << num << " is " << return_value;
        cout << "\nEnter another number(negative number to quit): ";
    }
    
    cout << "Program terminating."

    cin.clear();

    return 0;
}

int factorial(int n)
{
    int value = 0;
    if (n > 0)
    {
        value += (n * (n-1));
        --n;
        factorial(n);
    }
    else
        value = 1;

    return value;
}

[xibz]

Your logic is way off.
And your also missing a ';' at line 22.

int factorial(int n)
{
    if (n == 2)
        return 2;
    return n * factorial(n-1);
}

Basically this code just keeps multiplying n-1 until it gets to two, then returns 2.
What your code does is sets value to 0 every time factorial is called. So this already prevents problems with your algorithm. I have no idea why you're adding via line 34. This is not the definition of factorial.

I would also read up on scopes. Seems you're having trouble there.

Modified your code

int factorial(int n)
{
    static int value = n;
    if (n > 2)
    {
        value *= (n-1);
        --n;
        factorial(n);
    }

    return value;
}

Since this uses static though, it'll only compute the right number one time. So this algorithm still has issues, and I would use the simplest, which is the one I gave you above.

You could modify you algorithm to use a bool statement or something to check if it's running a new factorial. But I'll leave that up to you to think about.

[andywestken]

On line 36 of the orig code you are ignoring the return value from the recursive call to factorial(). Nor does it handle it in the modified code.

The whole point of recursion is that a function uses the value returned by a call to itself, with different parameters. Along with the termination condition, and error handling, that's about it.

Also note that the first version of factorial() in xibv's post does not correctly handle 0! = 1 or 1! = 1

[helios]

Also note that the first version of factorial() in xibv's post does not correctly handle 0! = 1 or 1! = 1

Not only that, factorial(x) where x<2 recurses indefinitely.

[georgewashere]

@xibz I will take your advice and read up on scopes, thank you for the suggestion.

@andywestken thank you for clarifying recursion for me.

I will return to the drawing board and see if I can work this out without copy and pasting solutions. Thank you all!


EDIT : posting my updated solution if anyone is interested, also need to transfer this code to my desktop p.c. for home review.

#include <iostream>
using namespace std;
int factorial(int);

int main()
{
    cout << "Enter a number you wish to get the factorial of: ";
    int num = 0;
    while (num >= 0)
    {
        int return_value;
        cout << "\n";
        cin >> num;
        if (num < 0)
            break;
        cout << "Calculating...";
        return_value = factorial(num);
        cout << "\nFactorial of " << num << " is " << return_value;
        cout << "\nEnter another number(negative number to quit): ";
    }

    cout << "Program terminating.";

    cin.clear();

    return 0;
}

int factorial (int n)
{
    int value = 1;

    if (n > 0 )
    {
        value = n * factorial(n-1);
    }

    return value;
}

[andywestken]

Cool!

Some people favour

int factorial (int n)
{
    if (n > 0 )
        return n * factorial(n-1);

    return 1;
}

BUT I can tell you that it makes no difference at all to the compiler output, in the release case.

(I like the version with the temp variable as it makes it easier to step through with the debugger.)

[xibz]

Haha yea, forgot about those numbers being less than 2. Good catch.