Is my code efficient?

Started by
4 comments, last by Serapth 10 years, 2 months ago

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

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.

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

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.

Tristam MacDonald. Ex-BigTech Software Engineer. Future farmer. [https://trist.am]

Not to be that crotchety old programmer but why...

At this stage in your career, why are you focused on speed? It's a mistake, one made far too often.

Instead you should probably should be focusing on writing clean code and worry about optimizations when the time comes. Truth of the matter is, the vast majority of code in any given project can be horrifically inefficient without the slightest bit of impact on the system as a whole. Nothing teaches optimization better than experience, so I would focus on that first.

So instead of asking "is this code efficient", ask is this code clean? In which case I would recommend cleaning up your loop variables as mentioned above, not using the horrifically unsafe printf/scanf, etc...

This topic is closed to new replies.

Advertisement