bubble sort on array of c structures not sorting properly

Question

I have the following program for books record and I want to sort the records on name of book. the code isn't showing any error but it's not sorting all the records.

#include "stdio.h"
#include "string.h"
#define SIZE 5

struct books{                                      //define struct
        char name[100],author[100];
        int year,copies;
    };

struct books book1[SIZE],book2[SIZE],*pointer;          //define struct vars

void sort(struct books *,int);                      //define sort func

main()
{
    int i;
    char c;

for(i=0;i<SIZE;i++)             //scanning values
{
    gets(book1[i].name);
    gets(book1[i].author);
    scanf("%d%d",&book1[i].year,&book1[i].copies);
    while((c = getchar()) != '\n' && c != EOF);
}
pointer=book1;
sort(pointer,SIZE);                 //sort call

i=0;                        //printing values
while(i<SIZE)
{
    printf("##########################################################################\n");
    printf("Book: %s\nAuthor: %s\nYear of Publication: %d\nNo of Copies: %d\n",book1[i].name,book1[i].author,book1[i].year,book1[i].copies);
    printf("##########################################################################\n");
    i++;
}
}

void sort(struct books *pointer,int n)
{
    int i,j,sorted=0;
    struct books temp;
for(i=0;(i<n-1)&&(sorted==0);i++)       //bubble sort on the book name
{
    sorted=1;
    for(j=0;j<n-i-1;j++)
    {
        if(strcmp((*pointer).name,(*(pointer+1)).name)>0)
        {
            //copy to temp val
            strcpy(temp.name,(*pointer).name);
            strcpy(temp.author,(*pointer).author);
            temp.year=(*pointer).year;
            temp.copies=(*pointer).copies;

            //copy next val
            strcpy((*pointer).name,(*(pointer+1)).name);
            strcpy((*pointer).author,(*(pointer+1)).author);
            (*pointer).year=(*(pointer+1)).year;
            (*pointer).copies=(*(pointer+1)).copies;

            //copy back temp val
            strcpy((*(pointer+1)).name,temp.name);
            strcpy((*(pointer+1)).author,temp.author);
            (*(pointer+1)).year=temp.year;
            (*(pointer+1)).copies=temp.copies;

            sorted=0;
        }
                *pointer++;
    }
}
}

My Imput

The C Programming Language
X Y Z
1934
56
Inferno
Dan Brown
1993
453
harry Potter and the soccers stone
J K Rowling
2012
150
Ruby On Rails
jim aurther nil
2004
130
Learn Python Easy Way
gmaps4rails
1967
100  

And the output

##########################################################################
Book: Inferno
Author: Dan Brown
Year of Publication: 1993
No of Copies: 453
##########################################################################
##########################################################################
Book: The C Programming Language
Author: X Y Z
Year of Publication: 1934
No of Copies: 56
##########################################################################
##########################################################################
Book: Ruby On Rails
Author: jim aurther nil
Year of Publication: 2004
No of Copies: 130
##########################################################################
##########################################################################
Book: Learn Python Easy Way
Author: gmaps4rails
Year of Publication: 1967
No of Copies: 100
##########################################################################
##########################################################################
Book: 
Author: 
Year of Publication: 0
No of Copies: 0
##########################################################################

We can see the above sorting is wrong? What I'm I doing wrong?

Answer 1

There are numerous problems with the code. The most blatant semantical error is that you are messing up the pointers (you increment the pointer in the inner loop but you never re-set it), so the code will invoke undefined behavior upon the second iteration by reading from and writing to unallocated memory.

The second problem is that you've got the bubble sort algorithm wrong -- if you are doing a bubble-up, then you have to run the outer loop from n - 1 to 0 downwards, else the first iteration won't have the chance to move the first (and potentially greatest) element in place. Subsequent iterations of the outer loop inherit the same behavior.

The rest of the problems are related to readability, style, design and safety. One is that you wrote (*pointer).member which should for the love of God be written as pointer->member. Another, similar one is (*(pointer + index)).member... That could just be pointer[index].member. The lack of proper formatting, indentation and whitespace is the third problem, using #include "" instead of #include <> for standard library headers is the fourth one.

Using gets() is a bad idea too, since it doesn't let you specify a buffer size which leads to potential buffer overflows. fgets() should always be preferred instead.

Code duplication and redundancy is bad -- you should always attempt procedural decomposition (this is the almost-blasphemous "refactoring" thing...) by pulling repetitive tasks (such as copying a books structure) out to their own functions. Naming variables is another important piece of design. Don't call something a pointer, that's not helpful to the reader (while reading your code, I was wondering, for a quite long time, as to what that pointer is a pointer to...)

You should also avoid global variables when possible. In your case, there is absolutely no need for any global variables whatsoever, so nothing really justifies their use. Another piece of good practice is to declare private helper functions as static so you decrease the risk of name collision.

The last fine point is: don't abuse comments. A line like

sort(books, SIZE); // call sort function

is not helpful. What else could sort(books, SIZE) possibly mean (in a well-designed code base anyway) than sorting the books array?

All in all, this is how I would have written the code:

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

#define SIZE 5

struct book {
    char title[100];
    char author[100];
    int year;
    int copies;
};

static void sort(struct book *books, int n);

int main()
{
    int i;
    int c; // EOF is out-of-range for `char`, this MUST be an int
    struct book books[SIZE];

    for (i = 0; i < SIZE; i++) {
        fgets(books[i].title, sizeof books[i].title, stdin);
        fgets(books[i].author, sizeof books[i].author, stdin);
        scanf("%d%d", &books[i].year, &books[i].copies);
        while ((c = getchar()) != '\n' && c != EOF);
    }

    sort(books, SIZE);

    for (i = 0; i < SIZE; i++) {
        printf("##########################################################################\n");
        printf("Book: %s\nAuthor: %s\nYear of Publication: %d\nNo of Copies: %d\n", books[i].title, books[i].author, books[i].year, books[i].copies);
        printf("##########################################################################\n");
    }
}

static void swap(struct book *a, struct book *b)
{
    struct book tmp = *a;
    *a = *b;
    *b = tmp;
}

static void sort(struct book *books, int n)
{
    int i, j;
    for (i = n - 1; i >= 0; i--) {
        for (j = 0; j < i; j++) {
            if (strcmp(books[j].title, books[j + 1].title) > 0) {
                swap(&books[j], &books[j + 1]);
            }
        }
    }
}