Return to Revisions

Your K&R bracing is flawlessly consistent, which is awesome =)

This is less so:

 if ( number == 0 )
 break;

You're consistently omitting the braces everywhere you can. While consistency is excellent, omitting optional braces has caused real, serious problems - in a nutshell:

if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0)
 goto fail;
 goto fail;

Don't goto fail - enclose all scopes with proper braces:

 if ( number == 0 ) {
 break;
 }

The whitespace inside the parentheses is unnecessary and feels distracting to me, and my own personal preference would be to remove the space between a function call and its argument list:

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

But as others have said this is nit-picking: consistency is king. Writing professional code is writing code that nicely blends into the existing code base, because it doesn't use a different bracing or spacing style. And it's consistently consistent.

Variables, unless they're trivial loop counters, should have a meaningful name. It is a well-established convention to use i and j in for loops, for example. But i in this case is more than a trivial loop counter: it's an argument to factorial.

In mathematics, the factorial of a non-negative integer \$n\$, denoted by \$n!\$, is the product of all positive integers less than or equal to \$n\$.

_{https://en.wikipedia.org/wiki/Factorial}

If you can't quite put a name on what that number \$n\$ would be called (I can't), IMO n would make a perfectly fine identifier for it. If there's a domain-specific name for it, it's probably a good idea to use that name instead. For example a divide function would preferably have dividend and divisor parameters, instead of x and y.

I like that there's a comment that immediately states that we're returning 0 when the return type is overflowed.

The problem is with returning 0 when the type is overflowed: it gives two different meanings to what the function returns - sometimes it's your result, and sometimes it's an error code. Yet using 0 for an error code doesn't feel right: typically a function that runs successfully to completion returns that value - and you already know that:

int main (void)
{
 //...
 return 0;
}

This Stack Overflow Q&A describes some good ways to deal with errors and return values.

If your function signature looked like this:

FactorialError factorial (int n, unsigned long long int *result)

It returns one of these:

typedef enum {NONE, OVERFLOW} FactorialError;

If all your functions consistently take a pointer to their result(s), and return an error value, then the day you have a function that needs to return different error codes, you don't need to start giving yet another meaning to what you're returning - and when you write code, the day you need to make a change to it is never really far - you could decide to make the function safe to call outside of main by handling a negative n parameter value and returning a different error code:

typedef enum {NONE, OVERFLOW, ILLEGAL_NEGATIVE} FactorialError;

(or in this case you could simply change the signature to take an unsigned int n.. that was just an example)

The beauty of this is that you don't need the comment anymore, and it becomes crystal-clear why the calling code needs that if block.. which, since we're returning an enum, would be better off as a switch block:

int DONE = 1;
int completed = 0;
while (completed == 0) {
 FactorialError result;
 unsigned long long int number;
 result = factorial(i, &number);
 switch (result) {
 case FactorialError.OVERFLOW:
 printf ("%2i! overflows the largest representable value.", i);
 completed = DONE;
 break;
 
 case FactorialError.ILLEGAL_NEGATIVE:
 printf ("%2i! cannot be computed.", i);
 completed = DONE;
 break;
 default:
 // assert result is FactorialError.NONE
 printf ("%2i %24llu\n", i, number);
 }
 ++i;
}

As for the actual implementation, as already mentioned returning early is never a bad idea; it removes the noise and lets you focus on the "clean" execution path:

// 0! is always 1
if (i == 0) {
 result = 1;
 return FactorialError.NONE;
}
// -1! cannot be computed
if (i < 0) {
 return FactorialError.ILLEGAL_NEGATIVE;
}
// 12! is the largest factorial that fits a 32-bit integer
if (i > 12) {
 return FactorialError.OVERFLOW;
}

Notice the error paths leave the pointer unassigned; reading that pointer in these error paths would be undefined behavior, which should be expected from the caller.

In this case we know that \$n!\$ needs to fit an int - assuming that's a 32-bit integer (I'm not familiar with c), the largest value of n we can work with is known to be 12 - so we can know we're going to overflow before we even start looping, leaving the body of the loop with a higher signal-to-noise ratio.

Arguably since the number of iterations is known from the start, a for loop would have been more appropriate than a while(1) infinite loop.