Factorial code in C

Question

It took me a while to figure out how to write this code, but I'm glad to say that I did eventually figure it out.

Can someone tell me what they think about it? Is it well written? What could have been done better?

Note that I am going through exercises in a book; I know there's a math.h that problem solves this, but I don't want to use that since I am not currently in that chapter.

#include <stdio.h>
int main(void)
{
printf("\n Table of Factorials\n\n");
printf(" n ------------- Factorial\n");
int n, factorial, counter; 
factorial = 1;
n = 1;
counter = 1;
for ( ; counter <= 10; ++counter)
{
 for ( ; n <= counter; ++n)
 { 
 factorial = factorial * n;
 }
 printf("%2i --------------%7i\n", counter, factorial);
}
return 0;
}

• CodesInChaos: I'd move the factorial computation into its own function.
• Erik Johansson: I hope you understands what happens if you try to get the factorial of 14 or maybe even 40? And why the simple fix isn't the right fix.
• Ruslan: I'm glad to see a non-recursive implementation of factorial.
• 200_success: Answers below have remarked on the indentation problems, but that seems to have been an artifact of copying-and-pasting the code into the text editor on this site. Markdown syntax requires four leading spaces to form a code block; the Markdown written in the question contained leading tabs instead.

Answer 1

Firstly, your indentation is a bit off. You should be indenting within a function body, so instead of:

int main(void)
{
printf(...);
// Other code
}

it should be:

int main(void)
{
 printf(...);
 // Other code
}

Similarly with:

for ( ; n <= counter; ++n)
{ 
factorial = factorial * n;
}

This is something to really watch out for; it makes for a jarring experience when reading the code.

The constant 10 here:

for ( ; counter <= 10; ++counter)

should be given a name:

#define MAX_COUNTER 10
for( ; counter <= MAX_COUNTER; ++counter)

The line:

factorial = factorial * n;

can be simplified into:

factorial *= n;

Having two loops here is more complex than it needs to be. It can be simplified to just using 1 loop. You could also pull the initialization of counter into this loop:

int counter;
for (counter = 1 ; counter <= MAX_COUNTER; ++counter)
{
 factorial *= counter;
 printf("%2i --------------%7i\n", counter, factorial);
}

If you're using C99, which allows variable declarations anywhere within a function (not just at the top), this can be simplified even more:

int factorial = 1;
for(int counter = 1; counter <= MAX_COUNTER; ++counter) {
 factorial *= counter;
 printf("%2i --------------%7i\n", counter, factorial);
}

• dyesdyes: Good, I was also wondering what the variable n was for. Maybe a personal idea but I think it's clearer to declare each variable on a single line.
• Kevin: @dyesdyes I agree, unless the variables belong to each other. For example: float x,y,z; or int screenWidth,screenHeight; That sort of thing.
• Charleh: Is it me or does the last simplification make no difference to the complexity or number of lines of code?
• Yuushi: @Charleh It limits variable scope to the smallest possible scope. In such a small function it makes almost no difference, but is a good habit to get into (assuming you're using a C99 compatible compiler, and aren't stuck with MSVC).
• Ruslan: Better yet would be const int MAX_COUNTER=10;. Doesn't change anything here, but generally it's better to make constants via const instead of #define.

Answer 2

Looks pretty good! Just a few minor things:

• Your variables should be on separate lines, which is useful for maintainability and possible commenting. They can also be initialized instead of declared and then assigned.

  int n = 1;
  int factorial = 1;
  int counter = 1;
  

• Instead of this:

  factorial = factorial * n;
  

  you can just have this:

  factorial *= n;
  

  This works for any of the mathematical operators.

• Instead of hard-coding the 10, you can have the user provide the number of values to display.

  puts("Enter the number of values to display: ");
  int numValues;
  scanf("%d", &numValues);
  for (int counter = 1; counter <= numValues; counter++)
  {
   // ...
  } 
  

Answer 3

Start by fixing your indentation. Lines of code inside each pair of braces should be indented by another level. I won't start any discussions about exactly how many spaces, but it's imperative that you do it consistently.

Your for-loop is missing the initializer field. That is legal in C, but you should consider it a red flag. In the case of your outer for-loop, you should just initialize counter there. Then you have a proper for-loop! In C99 (which is what nearly everyone uses these days), you can even move the int declaration into the for-loop initializer.

Next, tighten up your variable declarations. You can declare and define on the same line for better readability.

int main(void)
{
 printf("\n Table of Factorials\n\n");
 printf(" n ------------- Factorial\n");
 int factorial = 1;
 int n = 1;
 for (int counter = 1; counter <= 10; ++counter)
 {
 for ( ; n <= counter; ++n)
 {
 factorial = factorial * n;
 }
 printf("%2i --------------%7i\n", counter, factorial);
 }
 return 0;
}

I'm still not happy with that, though. Notice...

• Your table is supposed to display n and n!, but your printf() mentions counter.
• Your inner for-loop is still missing an initializer.

It turns out that your n and counter variables are redundant. This simpler program produces the same output:

int main(void)
{
 printf("\n Table of Factorials\n\n");
 printf(" n ------------- Factorial\n");
 int factorial = 1;
 for (int n = 1; n <= 10; ++n)
 {
 factorial *= n;
 printf("%2i --------------%7i\n", n, factorial);
 }
 return 0;
}

• Bob Jarvis - Слава Україні: Indentation scheme "discussions" often devolve into religious/flame wars. Most dev tools include a code formatter - set it up to your preference, use it (dare I say?) religiously, and co-exist. :-) Share and enjoy.