• Advertisement
Sign in to follow this  

Is my code efficient?

This topic is 1438 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

Hello, I made a simple program for my C class and just wanted to see what others thought about my code so I can improve. Is there a better way of doing what I was attempting? The program basically asks for an odd number which correlates to the size of the diamond which gets printed out

 

here's the code, any suggestions are appreciated! 

#include <stdio.h>
#include <stdbool.h>

int main(int argc, const char * argv[])
{
    int max_stars;
    bool odd = false;
    
    do {
    printf("How many stars wide do you want your diamond size to be? (odd): ");
    scanf("%d", &max_stars);
        
        // checks for odd number
        if(max_stars%2 == 1)
            odd = true;
    }while(odd == false);
    
    int num_spaces = max_stars - 1;
    int current_stars = 1;
    
    // top half of diamond
    for(int i = num_spaces; i > 0; i-=2) {
        for(int i = 0; i < num_spaces/2; i++)
            printf(" ");
        
        for(int i = 0; i < current_stars; i++)
            printf("*");
        
        printf("\n");
        current_stars += 2;
        num_spaces -= 2;
    }
    
    // bottom half of diamond
    for(int i = num_spaces; i <= max_stars; i+=2) {
        for(int i = 0; i < num_spaces/2; i++)
            printf(" ");
        
        for(int i = 0; i < current_stars; i++)
            printf("*");
        
        printf("\n");
        current_stars -= 2;
        num_spaces += 2;
    }
    
    return 0;
}
Edited by NUCLEAR RABBIT

Share this post


Link to post
Share on other sites
Advertisement

You could eliminate the odd variable like this: 

do {
    // ...stuff
} while (max_stars % 2 == 0);

Then you also don't need to inlcude stdbool.h

 

I think the condition is pretty self explanatory. Alternatively you could factor this expression out into a function is_odd()

 

Also you probably want to check if max_stars is greater than zero.

Edited by Madhed

Share this post


Link to post
Share on other sites

Your code will not compile in all compilers. You are using "int i" in every single for loop. The rest of the alphabet is jealous and wants to be included too.

 

I'm actually kind of surprised it compiles on any compiler. You're declaring "i" in inner loops while "i" is already in scope from the outer loop.

 

Other than that minor issue and the potential for negative inputs mentioned by madhed, everything looks good. 

Share this post


Link to post
Share on other sites

You could eliminate the odd variable like this: 

do {
    // ...stuff
} while (max_stars % 2 == 1);

Then you also don't need to inlcude stdbool.h

 

I think the condition is pretty self explanatory. Alternatively you could factor this expression out into a function is_odd()

 

Also you probably want to check if max_stars is greater than zero.

 

sweet thanks! made those tweeks!

 

 

Your code will not compile in all compilers. You are using "int i" in every single for loop. The rest of the alphabet is jealous and wants to be included too.

 

I'm actually kind of surprised it compiles on any compiler. You're declaring "i" in inner loops while "i" is already in scope from the outer loop.

 

Other than that minor issue and the potential for negative inputs mentioned by madhed, everything looks good. 

 

oops, didn't realize I was doing that haha thanks for pointing that out I shall make it more diverse! tongue.png

Share this post


Link to post
Share on other sites

Your code will not compile in all compilers. You are using "int i" in every single for loop. The rest of the alphabet is jealous and wants to be included too.
 
I'm actually kind of surprised it compiles on any compiler. You're declaring "i" in inner loops while "i" is already in scope from the outer loop.

Probably more detail than I should go into here, but: the code as written is no no danger of failing to compile. Although reusing the same variable name so many times is confusing (and leads to the potential for logic errors in this particular program), it is perfectly legal.

The declaration of 'int i' inside a for statement doesn't last beyond the end of the for loop, so redeclaring it in the next loops is both legal and necessary (note that older compilers will let the variable live on for the rest of the function, so not redeclaring it will generally work too).

Declaring 'int i' again in the inner for loops is also legal, as the inner declaration will hide the outer declaration. This means that changes to i inside the inner loop will affect the inner i, and outside of the inner loop will affect the outer i.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement