strcmp and strcpy is broken. I lost a word when i try to order them alphabetical in C

Question

right now I'm working on a bigger project. It's a long story, Santa Claus needs some help he has some cities and some toys to send. I need a function to order my cities alphabetical and i made one but, it worked fine for 9 tests and at the last one it bugged. Here's my function

void f_sort_city (structura_output s[], int n) {

int i, j;

structura_output AUX;

for (i = 0; i < n; i++) {
    for (j = i + 1; j < n; j++) {
        if (strcmp(s[i].city, s[j].city) > 0) {

            strcpy(AUX.city, s[i].city);
            strcpy(s[i].city, s[j].city);
            strcpy(s[j].city, AUX.city);

            strcpy(AUX.toy, s[i].toy);
            strcpy(s[i].toy, s[j].toy);
            strcpy(s[j].toy, AUX.toy);

            AUX.nr_toy = s[i].nr_toy;
            s[i].nr_toy = s[j].nr_toy;
            s[j].nr_toy = AUX.nr_toy;
        }
    }
}

}

if i print the city before i order them everything is fine but after i lost a city

here is my struct that i use

typedef struct {
char city[100];
char toy[100];
int nr_toy;
} structura_output;

cant show you my imput it has 400+ lines but the cities that i use are

ADDISA_ABABA MALABO

ALGER

YAMOUSSOUKRO

BUJUMBURA

DJIBOUTI

ASMARA

KINSHASA

BRAZZAVILLE

CAIRO

and my output is

ADDISA_ABABA

ALGER

ASMARA

BRAZZAVILLE

BUJUMBURA

CAIRO

>

DJIBOUTI

KINSHASA

MALABO

And i dont know hoe but i lost a city :\ Any kind of help would be awesome

how do you print your list? note: you could use `AUX = s[i];` etc... no need to swap all the fields, just use structure assignment, only 3 lines of code. — Jean-François Fabre, Jan 06 '18 at 22:50
@Jean-FrançoisFabre it worked with **AUX = s[i];** but now i'm losing a toy somehow :\ i mean i had 21 before and after i have 20 — Necula Horaţiu, Jan 06 '18 at 22:54
you need a [mcve] now. Even with 3 or 4 cities that could be enough to reproduce the issue. — Jean-François Fabre, Jan 06 '18 at 22:55
there's probably a memory corruption between the sort function and the display function, caused by some other code. — Jean-François Fabre, Jan 06 '18 at 22:55
@MartinJames it worked with 50 cities but i dont know why for this input it dosent work :\ — Necula Horaţiu, Jan 06 '18 at 23:18
There is no need for `strcpy` (or the individual copy of any members), simply assign to `AUX`, e.g. `AUX = s[i]; s[i] = s[j]; s[j] = AUX;` — David C. Rankin, Jan 07 '18 at 01:39

score 1 · Answer 1 · answered Jan 07 '18 at 02:01

You are not far off, but you are making things harder than they need to be. Since all members of your array are of fixed length, you can simply assign the structs. There is no need to do a deep copy copying each member. (that is needed where you have more than a single level of indirection in your members, like a pointer to pointer to type)

That shortens your f_sort_city function to:

void f_sort_city (structura_output s[], int n) {

    int i, j;

    structura_output AUX;

    for (i = 0; i < n; i++) {
        for (j = i + 1; j < n; j++) {
            if (strcmp(s[i].city, s[j].city) > 0) {
                AUX  = s[i];    /* no need to copy elements */
                s[i] = s[j];
                s[j] = AUX;
            }
        }
    }
}

Next, depending on how you read your data file (you didn't show the code), if you are reading with a line oriented read (e.g. fgets or POSIX getline) make sure you remove the trailing '\n' read an included by the function.

Don't use magic numbers in your code, e.g. char city[100];, etc.. If you need constants, #define them (or use an enum), e.g.

#define MAXC 100    /* don't use 'magic numbers in your code */
#define MAXS 128    /* if you need constants, define them.  */

Putting it altogether, you could implement your read (from stdin) and sort similar to the following:

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

#define MAXC 100    /* don't use 'magic numbers in your code */
#define MAXS 128    /* if you need constants, define them.  */

typedef struct {
    char city[MAXC];
    char toy[MAXC];
    int nr_toy;
} structura_output;

void f_sort_city (structura_output s[], int n) {

    int i, j;

    structura_output AUX;

    for (i = 0; i < n; i++) {
        for (j = i + 1; j < n; j++) {
            if (strcmp(s[i].city, s[j].city) > 0) {
                AUX  = s[i];    /* no need to copy elements */
                s[i] = s[j];
                s[j] = AUX;
            }
        }
    }
}

int main (void) {

    structura_output s[MAXS] = { {.city = ""} };
    size_t len, ndx = 0;

    while (ndx < MAXS && fgets (s[ndx].city, MAXC, stdin)) {
        len = strlen (s[ndx].city);                 /* get length */
        if (len && s[ndx].city[len - 1] == '\n')    /* test for newline */
            s[ndx].city[--len] = 0;                 /* trim newline */
        ndx++;  /* increment index */
    }

    f_sort_city (s, ndx);   /* sort data */

    for (size_t i = 0; i < ndx; i++)    /* output sorted data */
        printf ("s[%3zu] : %s\n", i, s[i].city);

    return 0;
}

(note: you should also check that a complete line of data was read by checking whether len + 1 == MAXC if the check for '\n' fails)

Example Input File

$ cat dat/cities.txt
ADDISA_ABABA MALABO
ALGER
YAMOUSSOUKRO
BUJUMBURA
DJIBOUTI
ASMARA
KINSHASA
BRAZZAVILLE
CAIRO

Example Use/Output

$ ./bin/sortcities < dat/cities.txt
s[  0] : ADDISA_ABABA MALABO
s[  1] : ALGER
s[  2] : ASMARA
s[  3] : BRAZZAVILLE
s[  4] : BUJUMBURA
s[  5] : CAIRO
s[  6] : DJIBOUTI
s[  7] : KINSHASA
s[  8] : YAMOUSSOUKRO

Look things over and let me know if you have further questions.

strcmp and strcpy is broken. I lost a word when i try to order them alphabetical in C

1 Answers1