HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Computing factorials

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
factorialscomputingstackoverflow

Problem

I am computing factorials as part of teaching myself C++. I am looking for feedback on efficiency, style and any bad practices I may be using.

#include 
#include 

using namespace std;

int main()
{
    unsigned int number{ 0 };
    unsigned long long factorial{ 0 };
    cout > number;
    while (cin.rdstate() || number  LLONG_MAX)
    {
        cin.clear();
        cin.ignore(LLONG_MAX, '\n');//flush input buffer
        cout > number;
    }
    if (number == 0 || number == 1)//since 0! and 1! = 1 there is no need to do a calculation
    {
        cout << "!" << number << " = 1" << endl;
    }
    else
    {
        factorial = number * (number - 1);
        for (size_t i = 2; i < number; ++i)
        {
            factorial *= (number - i);
        }
        cout << number << "! = " << factorial << endl;
    }
}

Solution

You should separate the computation code from the input/output code. As you have written it, the factorial computation code will never be reusable in any other program. To be specific, you should define an unsigned long long factorial(int n) function.

You have a formatting bug in the special case:

!0 = 1


In fact, you should structure your code to avoid the need for special cases altogether.

unsigned long long factorial(int n) {
    unsigned long long result = 1;
    while (n > 1) {
        result *= n--;
    }
    return result;
}

Code Snippets

unsigned long long factorial(int n) {
    unsigned long long result = 1;
    while (n > 1) {
        result *= n--;
    }
    return result;
}

Context

StackExchange Code Review Q#68709, answer score: 6

Revisions (0)

No revisions yet.