View more

View more

View more

Image of the Day Submit

IOTD | Top Screenshots

The latest, straight to your Inbox.

Subscribe to GameDev.net Direct to receive the latest updates and exclusive content.

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.

5 replies to this topic

#1NUCLEAR RABBIT  Members

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!

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.

#3richardurich  Members

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.

#4NUCLEAR RABBIT  Members

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!

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

#5swiftcoder  Senior Moderators

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] [GitHub]

#6Serapth  Members

Posted 14 February 2014 - 03:46 PM

POPULAR

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.