Qsort not sorting struct elements correctly

Question

I'm writing some code that reads in a binary file, sorts it, then writes it out to and output file sorted in lexicographical order. I'm not throwing any errors with any of it except for the sorting of the various elements.

struct lab1_data
{
    float goodbye;
    char balance;
    unsigned char clouds;
    float badge;
    double soda;
    char bat;
    short int parcel;
    char vessel;
    char spade;
    long int cover;
    unsigned long int hobbies;
    short int voyage;
    int stomach;
    char sort;
    char system [11];
    unsigned short  adjustment;
};


int compare(const void * a, const void * b)
{
    struct lab1_data** a1 = (struct lab1_data**) a;
    struct lab1_data** b1 = (struct lab1_data**) b;

    if ((*a1)->soda > (*b1)->soda)
    {
        return 1;
    }

    if ((*b1)->soda < (*b1)->soda)
    {
        return -1;

    }

    if ((*a1)->stomach > (*b1)->stomach)
    {
        return -1;
    }

    if ((*b1)->stomach < (*b1)->stomach)
    {
        return 1;

    }
    if ((*a1)->bat > (*b1)->bat)
    {
        return -1;
    }

    if ((*b1)->bat < (*b1)->bat)
    {
        return 1;

    }
    //This keeps going for all elements, just varying descending/ascending order.
    .
    .
    .
}

int main(int argc, char **argv)
{

    int size = 8;
    int count = 0;
    int i;

    struct lab1_data *lab1_struct;
    lab1_struct = (struct lab1_data*) malloc (size * sizeof(struct lab1_data));

    if (!lab1_struct)
    {
        fprintf(stderr, "Could not allocate memory");
        exit(-2) ;
    }

    FILE *fh_i = fopen(argv[1], "rb");
    FILE *fh_o = fopen(argv[2], "wb");

    if (argc != 3)
    {
        fprintf(stderr, "Incorrect file names");
        exit(1);
    }

    if (!fh_o)
    {
        fprintf(stderr, "Could not open file %s.", argv[2]);
        exit(-3);
    }
    if (!fh_i)
    {
        fprintf(stderr, "Could not open file %s.", argv[1]);
        exit(-3);
    }

    while (!feof(fh_i))
    {
        if (count == size)
        {
            size = size * 2;
            lab1_struct = realloc(lab1_struct, sizeof(struct lab1_data) * size);
        }

        fread((void*)&lab1_struct[count].goodbye, sizeof(float), 1, fh_i);
        fread((void*)&lab1_struct[count].balance, sizeof(char), 1, fh_i);
        fread((void*)&lab1_struct[count].clouds, sizeof(unsigned char), 1, fh_i);
        fread((void*)&lab1_struct[count].badge, sizeof(float), 1, fh_i);
        fread((void*)&lab1_struct[count].soda, sizeof(double), 1, fh_i);
        fread((void*)&lab1_struct[count].bat, sizeof(char), 1, fh_i);
        fread((void*)&lab1_struct[count].parcel, sizeof(short), 1, fh_i);
        fread((void*)&lab1_struct[count].vessel, sizeof(char), 1, fh_i);
        fread((void*)&lab1_struct[count].spade, sizeof(char), 1, fh_i);
        fread((void*)&lab1_struct[count].cover, sizeof(long), 1, fh_i);
        fread((void*)&lab1_struct[count].hobbies, sizeof(unsigned long), 1, fh_i);
        fread((void*)&lab1_struct[count].voyage, sizeof(short), 1, fh_i);
        fread((void*)&lab1_struct[count].stomach, sizeof(int), 1, fh_i);
        fread((void*)&lab1_struct[count].sort, sizeof(char), 1, fh_i);
        fread((void*)&lab1_struct[count].system, 11 * sizeof(char), 1, fh_i);
        fread((void*)&lab1_struct[count].adjustment, sizeof(short), 1, fh_i);

        count++;
    }

    struct lab1_data** plab1_struct = (struct lab1_data**) malloc (size * 8);
    for (i = 0; i < count; i++)
    {
        plab1_struct[i] = &lab1_struct[i];
    }

    qsort(plab1_struct, count - 1, 8, compare);

    for (i = 0; i < count; i++)
    {
        fwrite((void*) & (*plab1_struct[i]).goodbye, sizeof(float), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).balance, sizeof(char), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).clouds, sizeof(unsigned char), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).badge, sizeof(float), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).soda, sizeof(double), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).bat, sizeof(char), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).vessel, sizeof(short), 1 , fh_o);
        fwrite((void*) & (*plab1_struct[i]).parcel, sizeof(char), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).spade, sizeof(char), 1 , fh_o);
        fwrite((void*) & (*plab1_struct[i]).cover, sizeof(long), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).hobbies, sizeof(unsigned long), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).voyage, sizeof(short), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).stomach, sizeof(int), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).sort, sizeof(char), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).system, 11 * sizeof(char), 1, fh_o);
        fwrite((void*) & (*plab1_struct[i]).adjustment, sizeof(short), 1, fh_o);
    }

    fclose(fh_i);
    fclose(fh_o);

    free(lab1_struct);
    free(plab1_struct);

    return 0;
}

None of my elements are getting sorted at all. I was sure that I had the qsort functions called and initialised properly, I can't see what I'm missing.

You should do some debugging, and then construct a [minimal test-case](http://stackoverflow.com/help/mcve). — Oliver Charlesworth, Aug 26 '16 at 15:43
Note: They say [you shouldn't cast the result of `malloc()` in C](http://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). — MikeCAT, Aug 26 '16 at 15:45
Why did you cast to `void *` is this a c++ compiler? Also, `fread(&lab1_struct[count], sizeof(lab1_struct[count]), 1, fh_i);` is shorter don't you think? You just need to ensure the `struct` is packed, i.e. no padding. — Iharob Al Asimi, Aug 26 '16 at 15:45
How are the structures saved? Member by member as you read it, or the whole structure at once? Or the whole array of structures at once? — Some programmer dude, Aug 26 '16 at 15:45
@MikeCAT From the `fread((void *) ...` I would guess that the OP is using a [tag:c++] compiler. — Iharob Al Asimi, Aug 26 '16 at 15:46
`!feof(fh_i)` is [a bad loop condition](http://stackoverflow.com/questions/5431941/why-is-while-feof-file-always-wrong). — MikeCAT, Aug 26 '16 at 15:46
@iharob That only says how the OP is writing the data the OP just read, not how the data was written into the original file that the OP is reading from. — Some programmer dude, Aug 26 '16 at 15:47
Using a magic number `8` in `malloc (size * 8)` isn't a good idea. You should use `malloc (size * sizeof(struct lab1_data*))` instead. Using the magic number for the argument of `qsort` is also bad. — MikeCAT, Aug 26 '16 at 15:47
I know, but it's plausible that the OP is using the same kind of loop to save the original data. — Iharob Al Asimi, Aug 26 '16 at 15:48
Why are you sorting `count -1` elements instead of `count` elements? -- Ah, maybe due to the bad loop condition. — MikeCAT, Aug 26 '16 at 15:49
What is the purpose of the second array you allocate, the one with pointers to the elements in the first array? — Some programmer dude, Aug 26 '16 at 15:55
As for your problem, are you on a 32-bit or 64-bit system? Why aren't you using the `sizeof` operator to get the correct size for the third arguments? This will very much impact the behavior of your code. — Some programmer dude, Aug 26 '16 at 15:55
@MikeCAT yes but OP then goes on to use `count` in the next loop. — Weather Vane, Aug 26 '16 at 15:56
I don't understand why you have over-egged the pudding by making an array of `struct` pointers and sorting *them* instead of just sorting the structs themselves. Get that right first, and if it's too slow, then advance. BTW don't forget to `return 0;` at the end of the `cmp` function. — Weather Vane, Aug 26 '16 at 15:56
Finally, ***learn how to use a debugger!*** Even if you only want to become a hobby programmer, doing it for fun, knowing how to use a debugger and how to step through code line by line, break at certain lines, monitor variable values, knowing all those things are crucial. — Some programmer dude, Aug 26 '16 at 15:58
BTW you only need to allocate `count` elements in the pointer array, not `size` elements. — Weather Vane, Aug 26 '16 at 15:59

score 3 · Accepted Answer · answered Aug 26 '16 at 16:15

3

The compare function has this line which seems to be a typo as b1 is used twice:

    if ((*b1)->soda < (*b1)->soda)
          ^^            ^^

Same applies to:

    if ((*b1)->stomach < (*b1)->stomach)

    if ((*b1)->bat < (*b1)->bat)

answered Aug 26 '16 at 16:15

Support Ukraine

42,271
4
38
63

Good catch, there are several of those tests using the typo-ed pointer name. – Weather Vane Aug 26 '16 at 16:16

Qsort not sorting struct elements correctly

1 Answers1