void CREATE_ROW(int sprites_to_use, int points_to_use1, int points_to_use2, BITMAP *SCREEN_TO_USE)
{
     //SETUP
     int i, n, p, w, u, v;
     
     w = sprites_to_use;
     u = points_to_use1;
     v = points_to_use2;
     BITMAP *o = SCREEN_TO_USE;
     
     //DO THE MATH
     for(i = u; i <= v; i++){
        p = n;
        n = (rand() % w);
        if(p == n){
            if(n > (w - 1)){
                n--;
                }
            if(n <= (w - 1)){
                n++;
                }
            }
        draw_sprite(o, SP[n], points.x1, points.y1);
        slot.o = n;
        rest(50);
        }
}

What this loop does is create a row of sprites at certain points. the code works but sometimes it dosen't and i get a runtime error. The program just locks up. is there somehing im doing worng here? I am running vista with plenty of ram and hd space.

Hey, well the loop looks ok to me, the things that stick out though are the BITMAP *o pointer, if that's null somehow it could cause a runtime error, or the draw sprite function, where you refer to points, perhaps the index is going out of bounds? It may pay to do some checking on these variables before you use them, e.g.

if(o != NULL)
{
//Do stuff
}
else
{
//return error code
}

Also, can i suggest using more meaningful variable names? :)

Quote:Also, can i suggest using more meaningful variable names? :)

for some reason this made me laugh :P

I'm still fairly new to programing i just to the next big step form just learning c++ to allegro and 2d programing.

The reason I used all of those one letter variables was to save space on the fuction because i call it 6+ times during the program.

I did what you said and what do you know I built the game buffer wrong :P

But i think im going to write this fuction over in conherence with my struct's. I think it will make it run better that way.

Thanks for all your help!!!!

Quote:Original post by X_GRAYWOLF_X
The reason I used all of those one letter variables was to save space on the fuction because i call it 6+ times during the program.

The length of a variable name has nothing to do with execution time.

Quote:Original post by fpsgamer

Quote:Original post by X_GRAYWOLF_X
The reason I used all of those one letter variables was to save space on the fuction because i call it 6+ times during the program.

The length of a variable name has nothing to do with execution time.

Yeah, no effect. All the name is is "reference." When it compiles to machien code, it's going to be an address space anyway, regardless of the name length. The name is just for our convenience.

i think you 2 missed the point...

it's easier to write "o" 6+ times compared to "observationstarshipenterpriselegomanwalkingchinatown" 6+ times.

you're welcome.

Quote:Original post by nb
i think you 2 missed the point...

it's easier to write "o" 6+ times compared to "observationstarshipenterpriselegomanwalkingchinatown" 6+ times.

you're welcome.

and IMO you'll lose all that precious time you gained when you come back to debug your code and have no idea what you've done.

Quote:
observationstarshipenterpriselegomanwalkingchinatown

i'm a big fan of ctrl+c ctrl+v :)

Quote:Original post by nb
i think you 2 missed the point...

it's easier to write "o" 6+ times compared to "observationstarshipenterpriselegomanwalkingchinatown" 6+ times.

you're welcome.

I doubt this is what the OP is thinking, given things like this:

void CREATE_ROW(int sprites_to_use // <-- full variable name still used for the parameter{     u = points_to_use1; // assigned to a temporary, and then     for(i = u; i <= v; i++){ // *only used once*}

Let's clean up the code. I'll assume we're stuck with a C compiler, even though we really shouldn't. :)

Step 1: A variable that only gets written once and then read once is useless, except for breaking up a complex calculation (giving a name to a "subtotal"). It's also useless if it's read multiple times but still only written once first AND the "calculation" is trivial (i.e. just an assignment from another variable) - we could use the other variable instead.

The calculation 'w - 1' - excuse me, 'sprites_to_use - 1' - is not quite trivial, and the value is used twice, so you might think that we could benefit by making a variable to store that result. But if we look more carefully at how we're using it, we can see that we can (and should) simplify the code by just altering what kind of equalities we use.

void CREATE_ROW(int sprites_to_use, int points_to_use1, int points_to_use2, BITMAP *SCREEN_TO_USE){     //SETUP     int i, n, p;          //DO THE MATH     for (i = points_to_use1; i <= points_to_use2; i++){        p = n;        n = (rand() % sprites_to_use);        if(p == n){            if(n >= sprites_to_use){                n--;                }            if(n < sprites_to_use){                n++;                }            }        draw_sprite(SCREEN_TO_USE, SP[n], points.x1, points.y1);        slot.o = n;        rest(50);        }}

Step 2: Even in C, we can scope variables more tightly than the function level, so let's do that. I'll also remove the comments, since they don't actually tell us anything about what we're doing, and fix up the spacing a bit.

void CREATE_ROW(int sprites_to_use, int points_to_use1, int points_to_use2, BITMAP *SCREEN_TO_USE){    int i;    for (i = points_to_use1; i <= points_to_use2; i++) {        int n, p;        p = n;        n = (rand() % sprites_to_use);        if (p == n) {            if (n >= sprites_to_use) {                n--;            }            if (n < sprites_to_use) {                n++;            }        }        draw_sprite(SCREEN_TO_USE, SP[n], points.x1, points.y1);        slot.o = n;        rest(50);    }}

This highlights one of the problems with what we're doing - we assign 'p = n' right away, and the first time through the loop, 'n' has not been initialized. It looks like you're trying to avoid repeating the 'n' value on consecutive times through the loop, but there's a problem here. Let's look at the code for reassigning to 'n':

            if (n >= sprites_to_use) {                n--;            }            if (n < sprites_to_use) {                n++;            }

Well, 'n' came from the calculation '(rand() % sprites_to_use)', so before these checks happen, it must be less than sprites_to_use. So the first check never does anything. Then, in the second check, if n was picked to equal sprites_to_use - 1 (i.e. the last value), then it will get incremented, and end up out of range.

Reversing the two checks would prevent the crashes, but wouldn't achieve the desired effect: when the last slot is picked, the 'n' value would get incremented, and then decremented again, meaning that the value would be repeated after all.

To fix *that*, and be consistent with what happens in other cases, the natural choice would be to just wrap 'n' around to 0. But this still doesn't get it quite right, because now the results are biased: 'n' is twice as likely to be the "next" value after p - ((p + 1) % sprites_to_use) - as anything else (because that slot could either have been picked directly, or as a result of "correcting" a "bad" choice.



What we really want to do is pick one of the other slots. Since the "other" slots are every one *but* the one we currently have, there are therefore sprites_to_use - 1 of them, so we should pick a random number out of *that*. Then, to "map" that value onto the "other slots" is a little tricky, but still doable as shown. (If you can't follow along with the math as is, try this: grab a pop can, and cut a strip of paper just a little shorter than the circumference of the can. Write numbers 0...n in order on the paper and the can, with equal spacing, and play around with wrapping the paper around the can and lining up the numbers.

void CREATE_ROW(int sprites_to_use, int points_to_use1, int points_to_use2, BITMAP *SCREEN_TO_USE){    // The first time through, there is no "other slot", and we have a full    // set of options. So we need to handle it specially.    // Here we demonstrate initialization of a variable, as opposed to assignment.    int i = points_to_use1;    int previous = rand() % sprites_to_use;    draw_sprite(SCREEN_TO_USE, SP, points.x1, points.y1);    slot.o = previous;    rest(50);        // Note how 'i' is now one less than the first value we want to use in the    // loop, so we just increment.    for (i++; i <= points_to_use2; i++) {        // We pick a random number and then calculate what the next slot should be:        int current = (previous + (rand() % (sprites_to_use - 1)) + 1) % sprites_to_use;        // and set up for the next loop:        previous = current;        draw_sprite(SCREEN_TO_USE, SP[current], points.x1, points.y1);        slot.o = current;        rest(50);    }}

Stay tuned: that much should address the problems with the functionality, but there is still a lot more to learn about good style...

Episode 2.

In order to get things working last time, I had to handle the first point specially. This led to duplicating the code for drawing a sprite. Naughty! Of course, we should first extract a helper function for this:

void draw_sprite_in_row(BITMAP* screen, int point, int sprite) {    draw_sprite(screen, SP[sprite], points[point].x1, points[point].y1);    slot[point].o = sprite;    rest(50);}void CREATE_ROW(int sprites_to_use, int points_to_use1, int points_to_use2, BITMAP *SCREEN_TO_USE){    int i = points_to_use1;    int previous = rand() % sprites_to_use;    draw_sprite_in_row(SCREEN_TO_USE, i, previous);        for (i++; i <= points_to_use2; i++) {        int current = (previous + (rand() % (sprites_to_use - 1)) + 1) % sprites_to_use;        previous = current;        draw_sprite_in_row(SCREEN_TO_USE, i, current);    }}

Now we can start thinking a little more about how we've named things, since making a new function has automatically forced me ;) to set a good example.

- "_to_use" on a variable name doesn't really give us any extra information. Of course the variables are "to use"; why wouldn't we use them?

We don't want to shorten everything as far as possible, but we do want names that are *clear* and *expressive*. This means cutting out redundant stuff (while still using full words).

- When we take that out, we find that we are passing "points1" and "points2". Not very informative. They seem to represent the first and last values in a range of point IDs.

This kind of interface actually is not preferred by experienced programmers. It tends to lead to a lot of +1/-1 "corrections" in the code, and we don't like that (see the example from the previous post with 'sprites_to_use'). Sometimes, we pass a beginning and end point of a range, but it's usual to let the "end point" be just *past* our range; i.e. identifying the first thing *not* in the range beyond it. This makes our arithmetic work out nicely: the distance between endpoints equals the number of things.

If you're not comfortable with that, I suggest instead passing the "number of things" directly, along with the beginning value. This is the approach I will take here.

- Let's please not SHOUT without a good reason. We didn't get to name the BITMAP type, but we can certainly name its instances in a quiet, polite way. Don't emphasize things that aren't especially important.

void draw_sprite_in_row(BITMAP* screen, int point, int sprite) {    draw_sprite(screen, SP[sprite], points[point].x1, points[point].y1);    slot[point].o = sprite;    rest(50);}void create_row(int sprite_count, int first_point, int point_count, BITMAP* screen){    int i = first_point;    int previous = rand() % sprite_count;    draw_sprite_in_row(screen, i, previous);        // Notice that we now calculate the end value for the loop, and because    // we add a size value, we check for strict inequality.    for (i++; i < first_point + point_count; i++) {        int current = (previous + (rand() % (sprite_count - 1)) + 1) % sprites_to_use;        previous = current;        draw_sprite_in_row(screen, i, current);    }}

Next, a little analysis. It seems like we don't really need separate 'previous' and 'current' any more, given the order in which we set things. And it would be really nice to handle the first case inside the loop (that way we'll correctly do nothing when we are asked to make a row with zero points). The approach I'll take is to initialize the value first, and each time through the loop, use it and *then* change it. This way, it will always have an appropriate value when it's used (it gets changed in between each time), and the first time through, the value is selected properly.

void draw_sprite_in_row(BITMAP* screen, int point, int sprite) {    draw_sprite(screen, SP[sprite], points[point].x1, points[point].y1);    slot[point].o = sprite;    rest(50);}void create_row(int sprite_count, int first_point, int point_count, BITMAP* screen){    int i = first_point;    // 'sprite_id' still has to live "outside" the for-loop, because of its    // special-case initialization the first time. If we put it inside, we'd    // be repeating this initialization, which is not what we want.    int sprite_id = rand() % sprite_count;        // We no longer want to increment 'i', because the loop now handles all the    // sprites.    for (; i < first_point + point_count; i++) {        // First use the current value...        draw_sprite_in_row(screen, i, sprite_id);        // ... and then set up for the next loop.        // This means at the end, we calculate one more value that doesn't get        // used, but that is harmless.        // I'll make the change in two steps, to illustrate good use of the        // "immediate" arithmetic operators.        sprite_id += (rand() % (sprite_count - 1)) + 1;        sprite_id %= sprites_to_use;    }}

Notice that I don't "fold" the single-sprite-drawing function back in, even though it's only used once now. This is because it's helping us organize a bit: it gives a meaningful name to a process that's otherwise not so clear, and we might be able to reuse it somewhere else later (possibly with modification).