Shorten a sorting function

Question

I have this function:

int sort(book *data, int books, char mode, char sortBy) { int i, j; book aux; aux.cat = malloc(sizeof *(aux.cat)); if(sortBy == 'n') { for(i = 0; i < books; i++) { for(j = i; j < books; j++) { if(mode == 'a') { if(strcmp((data + i)->fname, (data + j)->fname) > 0) { aux = *(data + i); *(data + i) = *(data + j); *(data + j) = aux; } } else { if(strcmp((data + i)->fname, (data + j)->fname) < 0) { aux = *(data + i); *(data + i) = *(data + j); *(data + j) = aux; } } } } } else { for(i = 0; i < books; i++) { for(j = i; j < books; j++) { if(mode == 'a') { if(strcmp((data + i)->categ, "Fictiune") == 0) { if(((data + i)->cat)->price > ((data + j)->cat)->price) { aux = *(data + i); *(data + i) = *(data + j); *(data + j) = aux; } } } else { if(strcmp((data + i)->categ, "Fictiune") == 0) { if(((data + i)->cat)->price < ((data + j)->cat)->price) { aux = *(data + i); *(data + i) = *(data + j); *(data + j) = aux; } } } } } } free(aux.cat); return 0; }

I need to sort the data array in two modes: ascending and descending, and by two criteria: name and price.

The question is: is there a way to write another, more compact function that will do the same thing?

And, what do you think about line 5 (aux.cat = malloc(sizeof *(aux.cat));)? Is it a right way to initialize the cat pointer?

These are the structures:

typedef struct { int price, edition; char favCharacter[SMAX]; } category; typedef struct { char title[SMAX], fname[SMAX], lname[SMAX], categ[SMAX]; category *cat; } book;

200_success · Accepted Answer · 2014-01-25 21:42:47Z

The code for the two cases (sorting by name and sorting by price) have nearly nothing in common. You may be better off writing two functions. Even if you really like this interface, it might make sense to have the master function to call a sort-by-name function or a sort-by-price function to do the work.

I have no idea what aux.cat = malloc(sizeof *(aux.cat)) is for. You never really store anything in the .cat member. Furthermore, after all the swapping done in the sorting process, the aux.cat at the end of the function is not the same aux.cat that you started out with. That's a memory leak.

Your sort algorithm is selection sort, which takes O(n²) time. There are more efficient algorithms that work in O(n log n) time.

Your sort() function always returns 0. You might as well make it a void function and not return any value.

For efficiency and brevity, you would be better off using sorting routines built into the C library. All you have to do to use those routines is to define a comparator function, which tells the sorting function how to order any pair of books.

#include <stdlib.h> #include <string.h> static int compare_books_by_name_asc(const void *a, const void *b) { const book *book_a = (const book *)a, *book_b = (const book *)b; return strcmp(book_a->fname, book_b->fname); } static int compare_books_by_name_desc(const void *a, const void *b) { return compare_books_by_name_asc(b, a); } static int compare_books_by_price_asc(const void *a, const void *b) { int price_a = ((const book *)a)->cat->price, price_b = ((const book *)b)->cat->price; return price_a < price_b ? -1 : price_a > price_b ? +1 : 0; } static int compare_books_by_price_desc(const void *a, const void *b) { return compare_books_by_price_asc(b, a); } void sort(book *books, int num_books, char mode, char sortBy) { int (*comparator)(const void *, const void *); if (sortBy == 'n') { /* Sort by name */ comparator = (mode == 'a') ? compare_books_by_name_asc : compare_books_by_name_desc; } else { /* Sort by price */ comparator = (mode == 'a') ? compare_books_by_price_asc : compare_books_by_price_desc; } qsort(books, num_books, sizeof(*books), comparator); }

Stack Exchange Network

Shorten a sorting function

1 Answer 1

You must log in to answer this question.

Hot Network Questions

Shorten a sorting function

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions