C why is this for loop not executing when its conditions are met?

Question

I have a for loop which is used to find the nearest helicopter to a specific ship in danger.

The problem I am having is I need to ignore LifeBoats from my search (these have the type of L which is a single char in a struct) and only focus on Helicopters, represented by a H which is a single char in a struct.

The problem I have is that when I have this for loop:

closest_index = 0;

for (j = 0; j < asset_size; j++) {
        printf("type : %c \n", (assets + j)->type);
        if ((assets + j)->type == 'H') {
            if (((assets + j) ->distance_from_mayday) < ((assets + closest_index) ->distance_from_mayday)) {
                closest_index = j;
                printf("closest_index heli = %d \n", closest_index);

            }
        }
    }

It definitely gets called, I added the line:

 printf("type : %c \n", (assets + j)->type);

just before the comparison, and it produces this result in the console:

type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : L 
type : H 
type : H 
type : H 
type : H 
type : H 
type : H

As you can see there is values of H so I don't understand why this for loop is not executing as intented, any ideas?

Any particular reason you're using `(assets + j)->field` instead of `assets[j].field`? — Dietrich Epp, Nov 26 '13 at 23:49
"Not executing" is not an apt description. The log output shows that it *is* executing, but you're not getting the final result you want. — Potatoswatter, Nov 26 '13 at 23:55
So you've established that `(assets + j)->field` has the value `H` sometimes through the loop, but you still have another inner `if` condition besides that which must be met before it will finally do the inner `printf`. If the problem is that you're not seeing that output (I'm guessing that's what it is, since you've only said that the "for loop is not executing", which isn't the case) then I'd check that condition. — lurker, Nov 26 '13 at 23:58
yes, the assets "array" is a memory block of 37 structs its not strictly an array as it's defined elsewhere in a different source file and since you can't return arrays i adopted this approach. — chris edwards, Nov 26 '13 at 23:59
You certainly can return an array: http://stackoverflow.com/questions/1453410/declaring-a-c-function-to-return-an-array — Daniel, Nov 27 '13 at 00:05

ensc · Answer 1 · 2013-11-27T00:03:44.293

3

I guess, first element in list is of type 'L' and is lower or equal than any 'H' value. Your closest_index marker won't be moved hence.

It would be better to record the distance itself or use an impossible start value (-1?) for closest_index

EDIT:

Suggested code:

struct asset *result = NULL;

for (j = 0; j < asset_size; j++) {
     if (assets[j].type != 'H')
         continue;
     if (!result || assets[j].distance < result->distance)
         result = &assets[j];
}

edited Nov 27 '13 at 00:03

answered Nov 26 '13 at 23:54

ensc

6,704
14
22

Setting `closest_index = -1` would cause a buffer overflow on the first iteration, unless it's special-cased. – Potatoswatter Nov 26 '13 at 23:56
i have the same code, but comparing the value for 'L' and it works fine, the closest_index is implemented the same way, and functions correctly. it has to start at 0 so if the first element is the closest then the closest_index wont change, and the asset i am evaluating is controlled by J which does increment so i don't think its that issue :/ just an FYI element 7 has the lowest distance. – chris edwards Nov 26 '13 at 23:58
@Potatoswatter of course, you have to check for this (that's why I would record the distance and compare this) – ensc Nov 27 '13 at 00:00
1

@chrisedwards Generally it's better not to gain faith in a function's general correctness because it has functioned correctly under some conditions. Pointy-haired bosses can't be deterred from such attitude, but programmers generally can't afford it :) . – Potatoswatter Nov 27 '13 at 00:01
@chrisedwards true; [0] value does not need to be the lowest value at all but needs to be lower or equal than all 'H' values – ensc Nov 27 '13 at 00:02

score 1 · Accepted Answer · answered Nov 27 '13 at 00:00

Your code begins by assuming the first index in the array, in this case a life boat, is the closest helicopter to the event.

Maybe try something like this:

closest_index = 0;
closest_distance = INT_MAX;

for (j = 0; j < asset_size; j++) {
        printf("type : %c \n", assets[j]->type);
        if (assets[j]->type == 'H') {
            if (assets[j]->distance_from_mayday < closest_distance)  {
                closest_index = j;
                closest_distance = assets[j]->distance_from_mayday;
                printf("closest_index heli = %d \n", closest_index);
            }
        }
    }

If your list will always be sorted with the helis at the end (and you will always have at least one heli) then you could fix by changing your initial condition to:

closest_index = asset_size -1;

this worked, using a var to store the current closest value made me understand why i was going wrong, cheers Daniel! answer, accepted! — chris edwards, Nov 27 '13 at 00:19
FYI, adding termporary vars to find a problem is debugging 101. — Martin James, Nov 27 '13 at 00:53

Adam Liss · Answer 3 · 2013-11-27T00:14:20.107

The problem isn't the for; it's the if: none of your helicopters is closer than the first lifeboat. Here's one way to fix this:

closest_index = -1;

for (j = 0; j < asset_size; j++) {
  printf("type : %c\n", (assets + j)->type);
  if ((assets + j)->type == 'H') {
    if ((closest_index < 0) ||
        (assets + j)->distance_from_mayday <
            (assets + closest_index)->distance_from_mayday) {
      closest_index = j;
      printf("closest_index heli = %d\n", closest_index);
    }
  }
}

As a bonus, the loop will exit with closest_index == -1 if there are no helicopters.

If you care about the closest asset but not the index, you can simplify the loop as well:

Asset *closest_asset = NULL;

for (j = 0; j < asset_size; j++) {
  Asset *this_asset = assets + j;
  printf("type : %c\n", this_asset->type);
  if (this_asset->type == 'H' &&
      (closest_asset == NULL ||
       this_asset->distance_from_mayday < closest_asset->distance_from_mayday) {
    closest_asset = this_asset;
    printf("closest_index heli = %d\n", j);
  }
}

C why is this for loop not executing when its conditions are met?

3 Answers3