Simple text file I/O for book data

Question

I wanted to learn how to work with file I/O properly and found an assignment in my college papers and decided to write it:

//Write a C program which reads data about books and inputs them into a file named books.txt, like this:
//
//book_name#author_name#page_num#code#is_lent#
//
//book_name is an array of a maximum of 50 characters, author_name is an array of a maximum of 50 characters,
//page_num is an integer, code is an integer, is_lent is an integer of interval [0, 1] where 1
//means that the book is lent, and 0 that it isn't.
//Program creates a new file if it exists or opens an existing one and appends new
//data on the end. Input ends when the book name is 'x'.
//Print the contents of the file.

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

/*   Function prototypes    */
void buff_clr( void );
void write_to_file( FILE *fp );
void read_from_file( FILE *fp );
void remove_newline( char *string );

int main()
{
    FILE *fp;

    fp = fopen( "Books.txt" , "a+" );

    if( fp == NULL )
    {
        fprintf( stderr , "Error opening file" );
        exit( 1 );
    }

    write_to_file( fp );
    read_from_file( fp );
    fclose( fp );
    return 0;
}

/*   Removes newline from strings caused by fgets()    */
void remove_newline(char *string)
{
    char *ptr;

    if ( ( ptr = strchr( string , '\n' ) ) != NULL )
      *ptr = '\0';
}

/*   Picks up left over characters from the buffer in case there are any    */
void buff_clr(void)
{
    char garbage;
    do
    {
        garbage = getchar();
    }while( garbage != '\n' );
}

/*   Requests input and writes it to file   */
void write_to_file( FILE *fp )
{
    struct book_s
    {
        char book_title[50];
        char author_name[50];
        unsigned int page_num;
        unsigned int code;
        unsigned int is_lent;
    }book;

    while(1)
    {
        /*          Data input into struct        */
        printf( "Book title: " );

        fgets( book.book_title , 50 , stdin );

        remove_newline(book.book_title);

        if( strcmp( book.book_title , "x") == 0 )
        {
            break;
        }

        printf( "Name of the author: ");

        fgets( book.author_name , 50 , stdin );

        remove_newline(book.author_name);

        printf( "Number of pages: " );

        scanf( "%d" , &book.page_num );

        printf( "Book code: " );

        scanf( "%d" , &book.code );

        do
        {
            printf( "Is book lent(1 = yes , 0 = no):" );

            scanf( "%d" , &book.is_lent );
        }while(book.is_lent > 1);

        /*           Data input into file          */

        fprintf( fp , "%s#%s#%d#%d#%d#\n", book.book_title , book.author_name , book.page_num , book.code , book.is_lent );

        printf( "\n\n" );

        buff_clr(); //Using this here because without it a '\n' sneaks into book title on next iteration for some reason
    }
}

/*  Reads the file we've just written to    */
void read_from_file( FILE *fp )
{
    struct book_s
    {
        char book_title[50];
        char author_name[50];
        char page_num[5];
        char code[20];
        char is_lent[1];
    }book;

    char line[BUFSIZ];

    //Rewind to start of file
    fseek(fp, 0, SEEK_SET);

    while( fgets( line , sizeof(line) , fp ) != NULL )
    {
        strcpy( book.book_title , strtok( line , "#" ) );
        strcpy( book.author_name , strtok( NULL , "#" ) );
        strcpy( book.page_num , strtok( NULL , "#" ) );
        strcpy( book.code , strtok( NULL , "#" ) );
        strcpy( book.is_lent , strtok( NULL , "#" ) );

        printf( "\nBook title: %s", book.book_title );
        printf( "\nName of the author: %s", book.author_name );
        printf( "\nPage num: %s", book.page_num );
        printf( "\nBook code: %s", book.code );
        printf( "\nIs book lent: %s", book.is_lent );
        printf( "\n\n" );
    }
}

Now this all works within the confines of the assignment.

1. Is my code too long?
2. Could this have been written any shorter?
3. Could this have been written any better?
4. Did I miss anything?
5. Are there any mistakes?

A few things to note:

I know I don't really need structs but I had an idea of using them to create arrays of data and then print them one by one. I decided to scrap that idea since the only thing I'd accomplish is more lines of code and more memory wasted, but I liked the look of the struct so I kept it.

Is there a way I could read all the data in their rightful forms without having to read them all as a string and then parse them and then convert them (I didn't convert integers here since I didn't really need to)? I know I can't use fscanf() which would read strings until the whitespace character so it would try to gobble the whole line up since there aren't any.

Also, what if I had to read multiple lines of varying lengths and file types? For example, if instead of book_name#author_name#page_num#code#is_lent# being all one line, what if it was this?

book_name#page_num#
author_name#code#is_lent#

What would be the ways of reading such input?

Answer 1

Regarding correctness, read_from_file has some issues:

If the file content is malformed, and there is no item for e.g. the last strcpy line to read, then strtok will return null, and strcpy will try to copy a null pointer. Crash.

If the item in the file is larger than the strcpy destination buffers (the ones in your structure), then strcpy will keep going till it gets to the end of the destination string, overwriting other variables in your memory - a buffer overrun. This is a serius security flaw.

I suggest you use fscanf instead of memcpy and strtk.

Write to file:

I suggest rather than using a while(1), you go:

    printf("Press x to exit");
    char chrExitCommandIfX = getchr();

    while(chrExitCommandIfX != 'x') {

            ...

            printf("Press x to exit");
            char chrExitCommandIfX = getchr();
    }

Also, I suggest taking all the input first, as a dense block of fgetgs(), and then outputting the captured text for the user to confirm as a single printf statement. You can format the text like this for readability: http://dalelane.co.uk/blog/?p=88

But the comments for this function are my taste I guess.

More generally: Why not use a YAML libary? I mean, it a way that's a dumb question - you are figuring out file IO right? But I think 'is there a libary I could use for this' is a good question to be in the habit of asking...

Answer 2

I'm not a C guru, so just two small notes:

1. It does something strange (seems an endless loop) for the following input:

   Book title: a
   Name of the author: a
   Number of pages: a
   

2. The 50 magic number should be a named constant:

   char book_title[50];
   char author_name[50];
   

   The code uses them multiple times.

Answer 3

Here:

void buff_clr(void)
{
    char garbage;
    do
    {
        garbage = getchar();
    }while( garbage != '\n' );
}

You want to detect EOF otherwise the function might hang if there are no '\n' left. For this reason always use int to get the result of getchar().