Jump to content

  • Log In with Google      Sign In   
  • Create Account

Banner advertising on our site currently available from just $5!


1. Learn about the promo. 2. Sign up for GDNet+. 3. Set up your advert!


Is my code efficient?


Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.

  • You cannot reply to this topic
5 replies to this topic

#1 NUCLEAR RABBIT   Members   -  Reputation: 275

Like
0Likes
Like

Posted 14 February 2014 - 10:21 AM

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, 14 February 2014 - 10:26 AM.

------------------------My band: RISE OVER ME!

Sponsor:

#2 Madhed   Crossbones+   -  Reputation: 3467

Like
4Likes
Like

Posted 14 February 2014 - 10:33 AM

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, 14 February 2014 - 01:07 PM.


#3 richardurich   Members   -  Reputation: 1187

Like
3Likes
Like

Posted 14 February 2014 - 10:55 AM

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. 



#4 NUCLEAR RABBIT   Members   -  Reputation: 275

Like
0Likes
Like

Posted 14 February 2014 - 11:00 AM

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


------------------------My band: RISE OVER ME!

#5 swiftcoder   Senior Moderators   -  Reputation: 12897

Like
4Likes
Like

Posted 14 February 2014 - 12:00 PM

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 - Software Engineer @Amazon - [swiftcoding]


#6 Serapth   Crossbones+   -  Reputation: 5986

Like
5Likes
Like

Posted 14 February 2014 - 03:46 PM

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






Old topic!
Guest, the last post of this topic is over 60 days old and at this point you may not reply in this topic. If you wish to continue this conversation start a new topic.



PARTNERS