Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
}
share|improve this question
3  
I'd move the factorial computation into its own function. –  CodesInChaos Feb 12 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 at 12:30
    
I'm glad to see a non-recursive implementation of factorial. –  Ruslan Feb 12 at 14:10
    
@Yuushi I'm sorry, I didn't pay attention to the tag. –  Kevin Feb 12 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 at 4:24

3 Answers 3

up vote 17 down vote accepted

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);
}
share|improve this answer
    
Awesome how much simpler this is. Thank you! –  user3237895 Feb 12 at 7:27
3  
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 at 9:42
1  
@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 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 at 14:08
1  
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 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;
    
  • 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++)
    {
        // ...
    }        
    
share|improve this answer

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;
}
share|improve this answer
    
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 at 14:48

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.