3

Im trying to sort a array of strings I read from a file in alphabetical order using the qsort function. This is my code:

#include<stdio.h>
#include<stdlib.h>
#include<io.h>

#define MAXCHAR 256


int main(int argc, char **argv){
    char tempList[MAXCHAR][MAXCHAR];
    char reader[MAXCHAR];
    FILE* fp;
    int i;
    char *n;
    int index = 0;
    if(argc != 2){
        printf("too few arguments\n");
        exit(-1);
    }

    fp=fopen(argv[1], "r");
    if(fp == NULL){
        printf("failed to open file\n");
        exit(-1);
    }
    while(!feof(fp)){
        fgets(reader, MAXCHAR, fp);
        n = strchr(reader, '\n');
        if(n != NULL){
            *n = '\0';
        }
        strcpy(tempList[index], reader);
        index++;
    }
    index--;
    for(i=0; i<index; i++){
        printf("%s\n", tempList[i]);

    }
    qsort(tempList, index, sizeof(char *), strcmp);

    for(i=0; i<index; i++){
        printf("%s\n", tempList[i]);
    }
}

When I run the program, the list doesn't get sorted at all. I also tried methods posted on this website that asks a similar question and they all give me segmentation faults. Is there something wrong with the code?

Here is part of the content of the txt file. It is a list of 40 names:

Liam
Alexander
Mia
Noah
Emma
William
Charlotte
Charlotte
Mason
William
Ethan
Ethan
Liam
Alexander
Liam
Sophia
Emily
Mason
Alexander
Jay
  • 9,585
  • 6
  • 49
  • 72

3 Answers3

4

You have a bona fide array of arrays; not an array of char*. Arrays aren't pointers. qsort expects the stride of the elements in the sequence being sorted. As your sequence is declared as:

char tempList[MAXCHAR][MAXCHAR];

the proper element size is the size of the inferior element size. In this case you have an array of size MAXCHAR of array of char of size MAXCHAR (an array of arrays).

Thus this is wrong:

qsort(tempList, index, sizeof(char *), strcmp);
// wrong size ==============^^^^

the size of each element should be:

qsort(tempList, index, sizeof tempList[0], strcmp);
// correct size ==============^^^^

The other issues in your program will eventually grief you and are covered in general comments below your question, but this is the fundamental problem preventing your sorting from working correctly. A retooled version of your program appears below, addressing most of those concerns:

Updated Source

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#define MAXCHAR 256

/* properly declared for compatibility with qsort */
static int cmp_str(const void *lhs, const void *rhs)
{
    return strcmp(lhs, rhs);
}

/* main entrypoint */
int main(int argc, char **argv)
{
    char tempList[MAXCHAR][MAXCHAR];
    FILE* fp;
    size_t i, index = 0;

    if(argc != 2)
    {
        printf("too few arguments\n");
        return EXIT_FAILURE;
    }

    fp=fopen(argv[1], "r");
    if(fp == NULL)
    {
        perror(argv[1]);
        return EXIT_FAILURE;
    }

    while(index < MAXCHAR && fgets(tempList[index], sizeof(*tempList), fp) != NULL)
    {
        char *n = strchr(tempList[index], '\n');
        if(n != NULL)
            *n = 0;
        if (*(tempList[index])) /* don't insert blank lines */
            ++index;
    }

    for(i=0; i<index; i++)
        printf("%s\n", tempList[i]);
    fputc('\n', stdout);


    qsort(tempList, index, sizeof tempList[0], cmp_str);

    for(i=0; i<index; i++)
        printf("%s\n", tempList[i]);

    return EXIT_SUCCESS;
}

Untested, but it should be pretty close.

Best of luck.

WhozCraig
  • 65,258
  • 11
  • 75
  • 141
  • thanks! it works! quick questions though: 1) I heard its a bad practice to not brace after a if(), for(), while(), etc. statements but it seems like you (and alot of other programmers) still use it. was i misinformed when i heard its a bad practice? 2) is EXIT_FAILURE and EXIT_SUCCESS the same thing as exit(-1), exit(0)? are the keywords part of ? 3) dont i require ( in mac i believe) for file reading and writing? 4) is size_t another way of writing int? – NewbieProgrammer Aug 01 '15 at 11:25
  • 1
    1. Extremely opinionated question that that has pros and cons in both camps. But you should notice proper spacing and *indentation* are also important. 2. They're defined by the standard to be proper for your platform. That is why I use them. 3. This program uses all standard-library functions. no need for unix or mac-specific headers. 4. `size_t` is likewise defined by the standard as an unsigned type compatible with `sizeof`, etc. [Read here](http://en.cppreference.com/w/c/types/size_t) to learn more about it and its typical usage. – WhozCraig Aug 01 '15 at 16:31
2

Your size value in qsort(tempList, index, sizeof(char *), strcmp); is wrong. It should be qsort(tempList, index, sizeof(*tempList), strcmp);.

Robert Jacobs
  • 3,266
  • 1
  • 20
  • 30
  • In this case, the number of strings is the same as the string length, `MAXCHAR`. Better to do as @itsnotmyrealname answered and pass `sizeof(*tempList)` to `qsort`. Then if you change one or the other array size, this will spare any confusion. – Weather Vane Jul 31 '15 at 18:19
  • As you have edited the answer, I'll point out that `sizeof(*tempList)` is the same as `sizeof tempList[0]` as posted by @WhozCraig. – Weather Vane Jul 31 '15 at 18:29
1

I have tried to fix your code.

#include<stdio.h>
#include<stdlib.h>
#include<string.h>
//#include<io.h>    it's not standart


#define MAXCHAR 256

//  I implement the function because the warning is that
//  Incompatible pointer types passing 'int
//  (const char *, const char *)' to parameter of type
//  'int (*)(const void *, const void *)'

//  Already qsort() prototype is

// void qsort(void* ptr, size_t count, size_t size,
// int (*comp)(const void*, const void*));

// I think that the warning can be ignored strcmp also can be used

int myCompare(const void* a, const void* b)
{
    const char* aa = (const char*)a;
    const char* bb = (const char*)b;
    return strcmp(aa, bb);
}

int main(int argc, char **argv){
    char tempList[MAXCHAR][MAXCHAR];
    char reader[MAXCHAR];
    FILE* fp;
    int i;
    char *n;
    int index = 0;
    if(argc != 2){
        printf("too few arguments\n");
        exit(-1);
    }

    fp=fopen(argv[1], "r");
    if(fp == NULL){
        printf("failed to open file\n");
        exit(-1);
    }
    while(fgets(reader, MAXCHAR, fp) != NULL){ // !feof is not recommended search why

        n = strchr(reader, '\n');
        if(n != NULL){
            *n = '\0';
        }
        strcpy(tempList[index], reader);
        index++;
    }

    /*
    printf("%lu\n",sizeof(reader));     //256
    printf("%lu\n",sizeof(*reader));    //1
    printf("%lu\n",sizeof(*tempList));  //256
    printf("%lu\n",sizeof(**tempList)); //1
    */

    for(i=0; i<index; i++){
        printf("%s\n", tempList[i]);

    }
    qsort(tempList, index, sizeof(*tempList), myCompare);
    printf("\n\nAfter sorting\n\n");
    for(i=0; i<index; i++){
        printf("%s\n", tempList[i]);
    }
}
  • Would have been more educative to point out the errors, but as it gives the correct output (passing `strcmp` to `qsort` before your edit), +1. Why did you change it? – Weather Vane Jul 31 '15 at 17:40
  • Thanks for your advice, I've added as comment @WeatherVane – Soner from The Ottoman Empire Jul 31 '15 at 18:04
  • MSVC gave no warning about the arguments of the function passed to `qsort`. – Weather Vane Jul 31 '15 at 18:09
  • It's possible. I think the warning already can be ignored. But, XCode gives – Soner from The Ottoman Empire Jul 31 '15 at 18:12
  • WhozCraig's answer is better. It explains the actual problem (two-dimensional array of `char`, vs. array of `char *`). This answer has the right change to the args to qsort, but doesn't mention that change anywhere. – Peter Cordes Jul 31 '15 at 18:32
  • logic of actual problem can be deduced by looking through comments on my edited code. @PeterCordes – Soner from The Ottoman Empire Jul 31 '15 at 18:38
  • @itsnotmyrealname: that block of commented out printfs? Ok, that only makes sense after you pointed it out. It's not even next to the call to `qsort`. It's better to say explicitly and directly what the OP was missing, or else they'll often continue to miss it :/ – Peter Cordes Jul 31 '15 at 19:36
  • a person who is needer to fix his code shouldn't copy paste directly the right code. I think the person should look through the code(comments etc.) instead of directly copying. I do it because of the reason. Otherwise, I could also write obviously. @PeterCordes – Soner from The Ottoman Empire Jul 31 '15 at 19:46
  • @itsnotmyrealname: I'm not in favour of your idea of burying the real answer somewhere inside your post, to make sure they read everything you said. I'm saying I would explain *why* that line needed fixing, not *just* highlight it for them to copy/paste. – Peter Cordes Jul 31 '15 at 19:58
  • It is your idea. I stand behind my idea. @PeterCordes – Soner from The Ottoman Empire Jul 31 '15 at 20:07