Factorials, loops, break and professional code

The program below correctly calculates factorials, until the limit for an unsigned long long integer is exceeded.

My question has to do with loops and exiting loops. The program uses one infinite while loop and the break statement to exit 2 while loops.

Do working, professional programmers handle loops this way? If not, how would they write and exit these loops?

/* factorial.c

   program to calculate factorials until the limit for an unsigned long long
   integer is exceeded

   20! =    2,432,902,008,176,640,000
   limit = 18,446,744,073,709,551,615
   21! =   51,090,942,171,709,440,000
*/

#include 
#include 

unsigned long long int factorial (int i)
{
    // function returns 0 when limit exceeded

    int j;
    unsigned long long int result;

    if ( i == 0 )
        result = 1;
    else {
        result = i;    

        j = i - 1;

        while ( j >= 1 ) {
            if ( result <= ULLONG_MAX / j )    
                result *= j;
            else {
                result = 0;
                break;
            }

            --j;    
        }
    }    

    return result;
}

int main (void)
{
    int i;
    unsigned long long int number;

    printf ("\nTABLE OF FACTORIALS\n\n");
    printf (" n            n!\n");
    printf ("-- ------------------------\n");

    i = 0;

    while ( 1 ) {
        number = factorial (i);
        if ( number == 0 )
            break;       
        printf ("%2i %24llu\n", i, number);
        ++i;
    }

    return 0;
}

The other reviews have covered most items, so I'll just add a few.

Use constant string concatenation

The current code includes this:

printf("\nTABLE OF FACTORIALS\n\n");
printf(" n            n!\n");
printf("-- ------------------------\n");

But we don't really need to make 3 separate calls to printf here. Instead, let the compiler automatically concatenate string literals together so we can write this:

printf("\nTABLE OF FACTORIALS\n\n"
         " n            n!\n"
         "-- ------------------------\n");

I'd also suggest that puts would be a more appropriate call than printf since all we're printing is a string literal.

Be careful with signed vs. unsigned

If you don't intend for your code to handle negative numbers, I'd suggest that you indicate this clearly by changing the function signature to this:

unsigned long long factorial(unsigned i)

Minimize the scope of variable

In modern C, we don't tend to declare all variables at the top of each function. Instead, they're declared as they're needed and in as small a scope as possible, as in the following suggestion:

Prefer for to while loops

In the main code, the while(1) suggests that the loop continues forever but it really doesn't. In fact, we increment i each time and exit when the factorial code detects an overflow. Why don't we just write it that way instead of making the reader work so hard?

int main(void)
{
    printf("\nTABLE OF FACTORIALS\n\n"
             " n            n!\n"
             "-- ------------------------\n");

    unsigned long long number;

    // factorial() returns 0 on overflow
    for (unsigned i; (number = factorial(i)); ++i) {
        printf ("%2u %24llu\n", i, number);
    }
}

Simplify your code

The current factorial code has the same problem with its loop, which is that it's not made clear when the loop ends. Also, the code for detecting overflow is much more complex than it needs to be. Instead, we can simply note that when the multiplication results in overflow, the resulting product is less than the previous value. Using that, I'd write the code like this:

// return the factorial of i or
// 0 if multiplication overflows
unsigned long long factorial(unsigned i)
{
    if (i == 0) {
        return 1;
    }
    unsigned long long res;
    for (res = i--; i; --i) {
        unsigned long long prev = res;
        res *= i;
        if (res < prev) { // oveflow?
            return 0;
        }
    }
    return res;
}

Omit return 0

When a C or C++ program reaches the end of main the compiler will automatically generate code to return 0, so there is no need to put return 0; explicitly at the end of main.

Note: when I make this suggestion, it's almost invariably followed by one of two kinds of comments: "I didn't know that." or "That's bad advice!" My rationale is that it's safe and useful to rely on compiler behavior explicitly supported by the standard. For C, since C99; see ISO/IEC 9899:1999 section 5.1.2.2.3:


[...] a return from the initial call to the main function is equivalent to calling the exit function with the value returned by the main function as its argument; reaching the } that terminates the main function returns a value of 0.

For C++, since the first standard in 1998; see ISO/IEC 14882:1998 section 3.6.1:


If control reaches the end of main without encountering a return statement, the effect is that of executing return 0;

All versions of both standards since then (C99 and C++98) have maintained the same idea. We rely on automatically generated member functions in C++, and few people write explicit return; statements at the end of a void function. Reasons against omitting seem to boil down to "it looks weird". If, like me, you're curious about the rationale for the change to the C standard read this question. Also note that in the early 1990s this was considered "sloppy practice" because it was undefined behavior (although widely supported) at the time.

So I advocate omitting it; others disagree (often vehemently!) In any case, if you encounter code that omits it, you'll know that it's explicitly supported by the standard and you'll know what it means.

StackExchange Code Review Q#146809, answer score: 3