Qsort of dynamically allocated array of dynamically allocated strings by string length

Question

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

#define N 20

int compare(const void* a, const void* b) {
    return strlen((char*)a) - strlen((char*)b);
}

int main() {
    int i, n;
    scanf("%d", &n);
    char** strings = malloc(n*sizeof(char*));
    for(i=0; i<n;i++) {
        strings[i]=(char*)malloc(sizeof(char*));
        scanf("%s", strings[i]);
    }
    qsort(strings, n, sizeof(char*), compare);
    for(i=0; i<n;i++)
        printf("%s\n", strings[i]);
    for(i=0; i<n;i++)
        free(strings[i]);
    free(strings);
    return 0;
}

So I tried doing it like this but it returns a unsorted array and I got no idea what should be changed, anyone knows how to do this?

---

[update from comment:]

I forgot to mention, it should be sorted by length of strings.

Answer 1

From the C11 Standard (draft) on the qsort() function (emphases by me):

The contents of the array are sorted into ascending order according to a comparison function pointed to by compar, which is called with two arguments that point to the objects being compared.

The code shown wants to compare C-"strings", so the comparison function gets passed pointers to C-"strings", which are char** (here).

Your code treats the arguments as char *.

To fix this change:

int compare(const void* a, const void* b) {
  return strlen((char*)a) - strlen((char*)b);
}

to be:

int compare(const void * pv1, const void * pv2) {
  char * ps1 = *(char **) pv1;
  char * ps2 = *(char **) pv2;

  return strlen(ps1) - strlen(ps2);
}

Alternatively (to avoid the casts) do:

int compare(const void * pv1, const void * pv2) {
  char ** ppc1 = pv1;
  char ** ppc2 = pv2;

  return strlen(*ppc1) - strlen(*ppc2);
}

Note: Both the above snippets silently assume no element of strings is NULL.

---

Also when allocating to char* allocate chunks of size where char* points, that is *(char*), that is char.

So change this:

   strings[i]=(char*)malloc(sizeof(char*));

to be:

   strings[i] = malloc(sizeof(char));

of even better (as sizeof (char) is 1 be definition):

   strings[i] = malloc(1);

which leaves you with a "string" of 1 char only which allows you to store the empty-string ("") only.

You probably want N chars?

So do

   strings[i] = malloc(N + 1); /* 1+ for the 0-terminator. */

Note: In C there is no need to cast the result of malloc() (& Friends) nor is it recommended.

---

Finally make sure the user does not overflow the destination variable during input.

So you want to change this

  scanf("%s", strings[i]);

to be

  scanf("%20s", strings[i]);