patterncModerate
Factorial code in C
Viewed 0 times
codefactorialstackoverflow
Problem
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
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
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;
}Solution
Firstly, your indentation is a bit off. You should be indenting within a function body, so instead of:
it should be:
Similarly with:
This is something to really watch out for; it makes for a jarring experience when reading the code.
The constant
should be given a name:
The line:
can be simplified into:
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:
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 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);
}Code Snippets
int main(void)
{
printf(...);
// Other code
}int main(void)
{
printf(...);
// Other code
}for ( ; n <= counter; ++n)
{
factorial = factorial * n;
}for ( ; counter <= 10; ++counter)#define MAX_COUNTER 10
for( ; counter <= MAX_COUNTER; ++counter)Context
StackExchange Code Review Q#41447, answer score: 18
Revisions (0)
No revisions yet.