3
\$\begingroup\$

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?

\$\endgroup\$
3
\$\begingroup\$

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...

\$\endgroup\$
  • \$\begingroup\$ about the custom library.I forgot to mention that i'm learning this so i could help my friend pass his programming class. I would use a custom library if i could but since he can't i have to depend on default libraries. I'll include guards to check if strtok returns null. Using fscanf to read input? I tried that, however fscanf doesn't really work well for strings if the strings are first to be read. fscanf will read the string until the first whitespace character and thus it will try to read my whole line. \$\endgroup\$ – MrPlow Jun 25 '12 at 12:39
  • \$\begingroup\$ Actually fscanf's format strings are more flexible and complicated than that. You can use them as a kind of mini regex. For instance, you can read multiple lines, and use whatever charecter (e.g. #) to mark the end of a string. Most importantly, you can specify the max expected length of the strings %s to avoid running off the end of your buffers. I believe it stops reading at the apparant end of it's search string, rather than consuming to the next newline. It isn't wonderfully documented, but it's good to know. \$\endgroup\$ – AmaPseudonym Jun 25 '12 at 14:50
  • \$\begingroup\$ I tried writing something like fscanf(fp , "%50[^#]%*c%50[^#]%*c%d#%d#%d#" , .... ); To read the strings until the # character with [^#] and then discard the # character with %*c , however this did not work. The first variable contained garbage while others were empty. If i just used fscanf(fp, "%s#%s#%d#%d#%d#", ....) Then what i said earlier ,about reading the whole line as a string since there are no whitespace characters, would happen \$\endgroup\$ – MrPlow Jun 25 '12 at 15:31
  • \$\begingroup\$ Try this: pastie.org/4149470 I think the string you put there should work... but I got confused and tried to scanf %d into the page.booknum item in your structure, which was a char array. Trying to put a single number into an array as a whole = mess. Maybe you made the same mistake, or didn't read into a valuetype with & \$\endgroup\$ – AmaPseudonym Jun 25 '12 at 17:39
  • \$\begingroup\$ So i guess i messed up by using %*c to try and discard the #? I think when i tried scanf that i did change the appropriate struct members to int and that i used & but i can't be sure. Thanks for the correct code. Along with the correct way of using scanf you also gave me a few control ideas (i would never think of intSize and intScanResult variables to check if the file has any data and if scanf is reading it, i'd probably just use if scanf() == 5 so thanks for the ideas) \$\endgroup\$ – MrPlow Jun 25 '12 at 18:58
2
\$\begingroup\$

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.

\$\endgroup\$
  • 1
    \$\begingroup\$ 1. I forgot to put any guards to check if the input for page number is not an integer 2. I probably should have made it a constant \$\endgroup\$ – MrPlow Jun 24 '12 at 22:02
0
\$\begingroup\$

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().

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.