HiveBrain v1.2.0
Get Started
← Back to all entries
patterncModerate

Factorial code in C

Submitted by: @import:stackexchange-codereview··
0
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 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:

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.