# Factorial code in C

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;
}

-
I'd move the factorial computation into its own function. – CodesInChaos Feb 12 '14 at 9:51
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. – Erik Johansson Feb 12 '14 at 12:30
I'm glad to see a non-recursive implementation of factorial. – Ruslan Feb 12 '14 at 14:10
@Yuushi I'm sorry, I didn't pay attention to the tag. – Kevin Feb 12 '14 at 14:12
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. – 200_success Apr 9 '14 at 4:24

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);
}

-
Awesome how much simpler this is. Thank you! – user3237895 Feb 12 '14 at 7:27
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. – dyesdyes Feb 12 '14 at 9:42
@dyesdyes I agree, unless the variables belong to each other. For example: float x,y,z; or int screenWidth,screenHeight; That sort of thing. – Kevin Feb 12 '14 at 11:44
Is it me or does the last simplification make no difference to the complexity or number of lines of code? – Charleh Feb 12 '14 at 14:08
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. – Ruslan Feb 12 '14 at 14:12

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;


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++)
{
// ...
}

-

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;
}

-
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. – Bob Jarvis Feb 12 '14 at 14:48