qsort() and pointer problem

i'm trying to sort a array using qsort() but i just get junk printed. It probably has something to do with my pointers but i can't find what's wrong.

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "lab41.h"
person *personer[10];
int fillen;


int readfil(FILE *filen) {
    int counter = 0;
    int c;
    char mystring[100];
    person *p1;

    while((c = fgetc(filen)) != EOF) {
			        p1 = malloc(sizeof(person));
			        fscanf(filen,"%s", &p1->fnamn);
                    fscanf(filen,"%s", &p1->enamn);
            		fscanf(filen,"%d", &p1->alder);
            		fscanf(filen,"%d", &p1->vikt);
            		personer[counter] = p1;
            			
            			/*--------test ---------*/
            			
                        printf("%s", personer[counter]->fnamn);
                        printf(" ");
                        printf("%s", personer[counter]->enamn);
                        printf("\n");
                        printf("%d", personer[counter]->alder);
                        printf("\n");
                        printf("%d", personer[counter]->vikt);
                        printf("\n");
                        
                        /*--------slut test-------*/
                       counter++;
                       }
        fillen = counter;
		printf("\n---------------co--END---------------\n");
    return 0;
    }
    
int critter_cmp_namn (person *p1, person *p2)
{
  return strcmp (p1->fnamn, p2->fnamn);
}

    


int skriv_ut_personer(FILE *filen, int val) {
int counter = 0;
	while(counter < (fillen-1) ){
		if(val == 1) {
                        
                        fprintf(filen, "%s ", personer[counter]->fnamn);
                        
                        fprintf(filen, "%s\n", personer[counter]->enamn);
                        
                        fprintf(filen, "%d\n", personer[counter]->alder);
                        
                        fprintf(filen, "%d\n", personer[counter]->vikt);

		} else if (val == 0){
                          fprintf(stderr, "%s ", personer[counter]->fnamn);

                        fprintf(stderr, "%s\n", personer[counter]->enamn);

                        fprintf(stderr, "%d\n", personer[counter]->alder);

                        fprintf(stderr, "%d\n", personer[counter]->vikt);
			
		}
		counter++;
	}
  

return 0;
}

int meny() {
int val;
char filnamn[40];
FILE *filen;


printf("\nMENY\n");
printf("1. Sotera efter namn\n");
printf("2. Sotera efter alder\n");
printf("3. Sotera efter vikt\n");
printf("4. Skriv ut till fil\n");
printf("5. Skriv ut till terminal\n");
printf("Ditt Val: ");
scanf("%d", &val);


if(val == 1)
{
qsort (*personer, fillen, sizeof (person), critter_cmp_namn);
meny();
}
else if (val == 2)
{
}
else if(val == 3) {
} else if(val == 4) 
{
    printf("Fil att skriva till: ");
	scanf("%s", &filnamn);
	if((filen = fopen(filnamn, "w")) == NULL) {
             printf("går ej att skapa/öppna fil");
             meny();
        }
	skriv_ut_personer(filen, 1);
} 
else if(val == 5) {
skriv_ut_personer(NULL, 0);
} else {

	printf("felaktigt val");
	meny(filen);

}
return 0;
}

void main() {
int ett;
FILE *filen;
int val;
char filnamn[40];
enum filtyp {db,txt,bin};
typedef enum filtyp filtyp;
printf("Skriv filnamn: ");
scanf("%s", &filnamn);

if((filen = fopen(filnamn, "r")) == NULL) {
             printf("file don't exist");
	     exit(1);
        }
readfil(filen);

meny();
close(filen);
scanf("%d", ett);
}

Code:

qsort (*personer, fillen, sizeof (person), critter_cmp_namn);

should be

Code:

qsort (personer, fillen, sizeof (person), critter_cmp_namn);

Without knowing the typedef of person it's really hard to say, but you may want to go over these two assignments again:

Code:

fscanf(filen,"%s", &p1->fnamn);
fscanf(filen,"%s", &p1->enamn);

You should also be aware that main MUST return int, NOT void.

Code:

person *personer[10];

Code:

    qsort (*personer, fillen, sizeof (person), critter_cmp_namn);

Code:

int critter_cmp_namn (person *p1, person *p2)
{
    return strcmp (p1->fnamn, p2->fnamn);
}

change it to

Code:

person *personer[10];

Code:

    qsort (personer, fillen, sizeof(person *), (int (*)(const void *, const void *)) &critter_cmp_namn);

Code:

int critter_cmp_namn (person **p1, person **p2)
{
    return strcmp ((*p1)->fnamn, (*p2)->fnamn);
}

The code c.user gave is close, but is still not correct. It should be:

Code:

int critter_cmp_namn(const void *a, const void *b)
{
  person *const*p1 = a, *const*p2 = b;
  return strcmp((*p1)->fnamn, (*p2)->fnamn);
}
qsort(personer, fillen, sizeof(person *), critter_cmp_namn);

qsort() expects a particular type of function pointer, and simply casting the wrong type of pointer is not the right way to go.

Personally, I would prefer:

Code:

int critter_cmp_namn(const void *a, const void *b)
{
    const person *lhs = *(const person**)a;
    const person *rhs = *(const person**)b;
    return strcmp(lhs->fnamn, rhs->fnamn);
}
/* ... */
qsort(personer, fillen, sizeof(personer[0]), critter_cmp_namn);

My first idea is to remove one level of indirection to make it a little easier to interpret the strcmp() call (but then one can argue that the burden of interpreting the two levels of indirection has merely shifted to the casts). The second idea is to take the sizeof(personer[0]) instead of sizeof(person*), which makes for one less thing to modify should the element type of personer change.

My first idea is to remove one level of indirection to make it a little easier to interpret the strcmp() call (but then one can argue that the burden of interpreting the two levels of indirection has merely shifted to the casts). The second idea is to take the sizeof(personer[0]) instead of sizeof(person*), which makes for one less thing to modify should the element type of personer change.

I still can't decide which is "least worst". They both have their ugly qualities, but if my compare function has any sort of complexity, I'd likely do the same thing you did. Better to have one ugly initialization than ugly use all over the function.

As for applying sizeof to the object rather than the type, I concur 100%. I can't really think of a good reason to use sizeof with a type name if it's not necessary.

I will correct my function a bit

Code:

int critter_cmp_namn (person **p1, person **p2)
{
    if (!p1 || !p2 || !*p1 || !*p2)
        return 0;
    return strcmp ((*p1)->fnamn, (*p2)->fnamn);
}

it would be better

Originally Posted by c.user

I will correct my function a bit

Code:

int critter_cmp_namn (person **p1, person **p2)
{
    if (!p1 || !p2 || !*p1 || !*p2)
        return 0;
    return strcmp ((*p1)->fnamn, (*p2)->fnamn);
}

it would be better

Actually it would be terrible, because it returns 0 for NULL pointers, AND for a match.


Quzah.

Originally Posted by c.user

I will correct my function a bit

Code:

int critter_cmp_namn (person **p1, person **p2)
{
    if (!p1 || !p2 || !*p1 || !*p2)
        return 0;
    return strcmp ((*p1)->fnamn, (*p2)->fnamn);
}

it would be better

That looks to me like that if-statement is attempting to do the job of an assert, so you should probably just use an assert.

I wouldn't think you intend the ordering of NULL vs non-NULL item to be non-transitive. Imagine 3 items (a, b, and c). b is NULL, but a and c are not, and a is less than c.
Comparisons of a,b, and c could lead us to conclude that b < b. Oops!
The result of course is that the sorting function will probably not produce the desired result. In fact some sorting algorithms that rely on transitivity could get confused and even crash under those circumstances.

I tryed to leave the NULL in it's place (so now I see that the sorting will not correct though, hence it should return 1 for NULL and it is necessary to make something like errno (but local in the program) for the informing the programmer about this situation)
then the programmer can interpret it as an error or not

Originally Posted by c.user

I tryed to leave the NULL in it's place (so now I see that the sorting will not correct though, hence it should return 1 for NULL and it is necessary to make something like errno (but local in the program) for the informing the programmer about this situation)

As a pre-condition to that comparator, null pointers should not be allowed. It is needlessly inefficient to check if the arguments are null pointers each time the function is called. As iMalc noted, an assert should be used to check the pre-condition, upon which the check can be turned off for a release build if necessary, e.g.,

Code:

int critter_cmp_namn(const void *a, const void *b)
{
    const person *lhs = *(const person**)a;
    const person *rhs = *(const person**)b;
    assert(a && b && "Null pointers not allowed as arguments to person comparator.");
    return strcmp(lhs->fnamn, rhs->fnamn);
}

It is probably better to place the assert before the dereference, but I am assuming C90 instead of C99.

There is no point checking for empty strings separately since strcmp() will already handle that case.

Code:

char nullflag;

...

int critter_cmp_namn (person **p1, person **p2)
{
    if (!p1 || !p2 || !*p1 || !*p2)
        return nullflag = 1;
    return strcmp ((*p1)->fnamn, (*p2)->fnamn);
}

then after sorting

Code:

    if (nullflag) {
        nullflag = 0;
        /* assert(nullflag && "Null pointers are not allowed in the sorting"); */
        for (personp = personer; personp != NULL; personp++)
            Print(personp);
        printf("with nulls\n");
    } else {
        int i;
        
        for (i = 0; i < sizeof personer / sizeof *personer; i++)
            Print(&person[i])
    }

i.e. he can decide what to do

Originally Posted by laserlight

As iMalc noted, an assert should be used to check the pre-condition, upon which the check can be turned off for a release build if necessary, e.g.

I am recommend to control the program, and you are recommend to debug the program
if it will be turned off what will happen if there will an incorrect data in that array ?