6
\$\begingroup\$

The assignment is very open and we have only 4 things it needs to cover:

  1. File write and read
  2. Use a Struct
  3. Export into HTML format
  4. use a common sorting algorithm

so I've decided to create a little "Library" with following features:

  • read the input file "Library.dat"
  • List the books on the console
  • Sort the books by name
  • Add new book
  • delete a book by ID
  • check a book in / out

So I'm a beginner at C and would be very happy for a review and some tips on how to improve my project.

Input File:

1;v;121212;121212;1
2;a;121212;121212;0
4;e;121212;121212;1
6;d;121212;121212;1
7;w;121212;121212;0
8;x;121212;121212;1
9;c;121212;121212;1

My Config.h file:

//
//  Library.config
//
//  Created by Me on 03.12.13.
//  Copyright (c) 2013 Me. All rights reserved.
//

// DEFINE

#define MAX_STR_LEN 256
#define MAX_BOOKS 10
#define DATE_LEN 8
#define PATH "/users/xxxxxxx/desktop/Library/Library.dat"

My Code:

/*************************************************
 * Library Application
 *
 * Author:  Me
 * Date:    26.11.2013
 * Version: BETA
 *
 *
 *************************************************/

/****************************
 *  OSX: fpurge(stdin) insted of fflush(stdin)
 *
 *
 *
 *****************************/



/* TODOs:
 *
 * Search Function
 * Colors (conio.h)?
 *
 *
 */

/* header files for access to specialized functions*/
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include "config.h"



typedef struct {
    int ID;
    char name[MAX_STR_LEN];
    char dateIn[DATE_LEN];
    char dateOut[DATE_LEN];
    int isIn;
}book;

struct book{
    int ID;
    char name[MAX_STR_LEN];
    char dateIn[DATE_LEN];
    char dateOut[DATE_LEN];
    int isIn;
};


/* array of my books */
struct book books[MAX_BOOKS];

/* PROTOTYPE OF FUNCTIONS */
int readBookFile();
void printBookList();
void addBook();
void mutateBook(book b);    //TODO
int getNextID();
book enterBookData();
void delBook();
int writeBookFile();
void menu();
void checkOut();
void checkIn();
void sort();

/****************************************************
 * Function: main()
 *
 * Description: What is there to explain..
 *
 * Parameters:  argv (the string of all parameters),
 *              argc (the count of all input aruments)
 *
 * return parameters: int, 0 = OK, 1 = Error
 ****************************************************/
int main(int argc, char **argv)
{
    /* declaration of the return parameter */
    int isOK = 0;

    /* first read the input file for the Library */
    isOK = readBookFile();

    /* the menu is the main controll structure */
    menu();

    /* last write the changes made to the Library file */
    isOK = writeBookFile();

    /* pause the system, so we see its over */
    //system("pause");
    system( "read -n 1 -s -p \"Press any key to continue...\"" );


    /* return 1 = Error, 0 = ok */
    return isOK;
}


/*********************************************
 * Function: readBookFile()
 *
 * Description: Read the csv file into a books arrray
 *
 * Parameters: none
 *
 * Return parameters: none
 * ********************************************/
int readBookFile()
{
    /* FileStream for the Library File */
    FILE *bookFile;

    /* allocation of the buffer for every line in the File */
    char *buf = malloc(MAX_STR_LEN);
    char *tmp;

    /* if the space could not be allocaed, return an error */
    if (buf == NULL) {
        printf ("No memory\n");
        return 1;
    }

    /* chack if the file can really be loaded into the stream */
    if ( ( bookFile = fopen(  PATH , "r" ) ) == NULL ) //Reading a file
    {
        printf( "File could not be opened.\n" );
        return 1;
    }

    int i = 0;
    /* read every line of the inputfile to an element of the books array */
    while (fgets(buf, 255, bookFile) != NULL)
    {
        /* this is to check if the files last line was reached last turn */
        if(ferror(bookFile))
        {
            break;
        }

        /* if we don't chech for newline we skip every second line */
        if ((strlen(buf)>0) && (buf[strlen (buf) - 1] == '\n'))
            buf[strlen (buf) - 1] = '\0';

        /* read the ID and cast it to an Integer  */
        tmp = strtok(buf, ";");
        books[i].ID = atoi(tmp);

        /*  to give the value of the tmp part of the csv string
            to books[i] we need to copy the string! */
        tmp = strtok(NULL, ";");
        strcpy( books[i].name, tmp);

        tmp = strtok(NULL, ";");
        strcpy(books[i].dateIn, tmp);

        tmp = strtok(NULL, ";");
        strcpy(books[i].dateOut, tmp);

        tmp = strtok(NULL, ";");
        books[i].isIn = atoi(tmp);

        //      printf("index i= %i  ID: %i, %s, %s, %s \n",i, books[i].ID , books[i].name, books[i].dateIn , books[i].dateOut);
        /* increment for next book in books[] */
        i++;

    }

    /* free the buffer */
    free(buf);

    /* close the filestream */
    fclose(bookFile);
    return 0;
}

/*********************************************
 * Function: printBookList()
 *
 * Description: Print the Array of books to the console
 *
 * Parameters: none
 *
 * Return parameters: none
 * ********************************************/
void printBookList()
{

    int i;
    sort();
    printf("ID: \t  Title:  \t\t  Dateout:  \tDate in:\tOut? \n");
    printf("------------------------------------------------------------------------\n");
    for (i = 0; i <= MAX_BOOKS; i++)
    {
        /* only do this until the last is printed, as 256 are allocated by default */
        if (books[i].ID != 0)
        /* the %.15s means only the first 15 characters of the string will be printed  */
            printf("ID: %i\t  %.15s  \t  %s  \t%s\t\t%i \n", books[i].ID , books[i].name, books[i].dateIn , books[i].dateOut, books[i].isIn);
        else
            break;
    }
    printf("------------------------------------------------------------------------\n");

}

/*********************************************
 * Function: addBook()
 *
 * Description: add a new book to the books[] array
 *
 * Parameters: none
 *
 * Return parameters: none
 * ********************************************/
void addBook()
{
    /* the function enterBookData return the new book */
    book bin = enterBookData();

    int newID = 0;
    /* the getNextID returns the next free ID number */
    newID = getNextID();

    books[newID].ID= newID + 1;
    strcpy(books[newID].name, bin.name);
    strcpy(books[newID].dateIn, bin.dateIn);
    strcpy(books[newID].dateOut, bin.dateOut);
    books[newID].isIn =  bin.isIn;

}


/*********************************************
 * Function: getNextID()
 *
 * Description: returns the next free index of the books[] array
 *
 * Parameters: none
 *
 * Return parameters: int, the next free index of books[]
 * ********************************************/
int getNextID()
{
    int i;

    for (i = 0; i <= MAX_BOOKS; i++)
    {
        if (books[i].ID == 0)
            break;
    }
    return i;
}


/*********************************************
 * Function: enterBookData()
 *
 * Description: get the user input of every Attribute
 *
 * Parameters: none
 *
 * Return parameters: the new book
 * ********************************************/
book enterBookData()
{
    book b1 = {0,"","",""};
    char tmp[MAX_STR_LEN];

    printf("Titel: ");
    fgets(tmp, MAX_STR_LEN, stdin);

    if ((strlen(tmp)>0) && (tmp[strlen (tmp) - 1] == '\n'))
        tmp[strlen (tmp) - 1] = '\0';

    strcpy(b1.name, tmp);

    /* free the standard in, ohterwise you will encounter problems if the enterd string is too long  */
    fpurge(stdin);

    printf("datein (DDMMYY):");
    fgets(tmp, DATE_LEN, stdin);

    if ((strlen(tmp)>0) && (tmp[strlen (tmp) - 1] == '\n'))
        tmp[strlen (tmp) - 1] = '\0';

    strcpy(b1.dateIn, tmp);

    fpurge(stdin);

    printf("dateout (DDMMYY): ");
    fgets(tmp, DATE_LEN, stdin);

    if ((strlen(tmp)>0) && (tmp[strlen (tmp) - 1] == '\n'))
        tmp[strlen (tmp) - 1] = '\0';

    strcpy(b1.dateOut, tmp);

    fpurge(stdin);

    printf("is it in? (1 = yes, 0 = no): ");
    fgets(tmp, 2, stdin);

    if ((strlen(tmp)>0) && (tmp[strlen (tmp) - 1] == '\n'))
        tmp[strlen (tmp) - 1] = '\0';

    b1.isIn=  atoi(tmp);

    fpurge(stdin);

    return b1;
}

/*********************************************
 * Function: delBook()
 *
 * Description: deletes the index of the books[] array the user choose
 *
 * Parameters: none
 *
 * Return parameters: none
 * ********************************************/
void delBook()
{
    char ids[3];
    int id = 0, c;
    printf("Enter linenumber of book to delete. \n");

    fgets(ids, MAX_STR_LEN, stdin);

    if ((strlen(ids)>0) && (ids[strlen (ids) - 1] == '\n'))
        ids[strlen (ids) - 1] = '\0';

    id= atoi(ids);
    if (id > MAX_BOOKS)
        id = MAX_BOOKS;

    for ( c = id - 1 ; c < MAX_BOOKS - 1 ; c++ )
        books[c] = books[c+1];

    fflush(stdin);
}

/*********************************************
 * Function: writeBookFile()
 *
 * Description: writes the array of books[] to the file (overwrites)
 *
 * Parameters: none
 *
 * Return parameters: int, could the file be written? 1 = no, 0 = yes
 * ********************************************/
int writeBookFile()
{
    /* FileStream for the Library File */
    FILE *bookFile;

    if ( ( bookFile = fopen( PATH, "w" ) ) == NULL ) //Reading a file
    {
        printf( "File could not be opened.\n" );
        return 1;
    }

    int i;
    for(i = 0; i < MAX_BOOKS;i++)
    {

        if(books[i].ID != 0)
        {
            fprintf(bookFile, "%i;%s;%s;%s;%i\n", books[i].ID, books[i].name, books[i].dateIn, books[i].dateOut, books[i].isIn);
        }
    }

    fclose(bookFile);
    return 0;
}

/*********************************************
 * Function: menu()
 *
 * Description: the main menu, logic and printfs
 *
 * Parameters: none
 *
 * Return parameters: none
 * ********************************************/
void menu()
{
    char choice[1];

    while(choice[0] != '5')
    {
        system("clear");
        //printf("\33[2J");
        printBookList();

        printf("1. Add book    \n");
        printf("2. Delete book \n");
        printf("3. Check out book \n");
        printf("4. Check in book \n");
        printf("5. Quit        \n");
        printf("Please enter the number of your choice: ");
        fgets(choice, 2, stdin);
        fpurge(stdin);

        switch(choice[0])
        {
            case '1':
            {
                addBook();
                break;
            }

            case '2':
            {
                delBook();
                break;
            }

            case '3':
            {
                checkOut();
                break;
            }

            case '4':
            {
                checkIn();
                break;
            }

        }

    }

}

/*********************************************
 * Function: checkOut()
 *
 * Description: To check out a book, the user chooses the ID and then sets the isIn attribute of the book to 0
 *
 * Parameters: none
 *
 * Return parameters: none
 * ********************************************/
void checkOut()
{
    char tmp[3];
    int i = 0;
    printf("Please enter the ID of the book beeing checked out: ");

    fgets(tmp, 4, stdin);

    if ((strlen(tmp)>0) && (tmp[strlen (tmp) - 1] == '\n'))
        tmp[strlen (tmp) - 1] = '\0';

    fflush(stdin);

    for (i = 0; i < MAX_BOOKS;i++)
    {
        if(books[i].ID == atoi(tmp))
        {
            if (books[i].isIn == 0)
            {
                printf("Book allready checked out!\n");
                system("pause");
            }
            else
            {
                books[i].isIn = 0;
            }
            break;
        }

    }


}

/*********************************************
 * Function: checkIn()
 *
 * Description: To check in a book, the user chooses the ID and then sets the isIn attribute of the book to 1
 *
 * Parameters: none
 *
 * Return parameters: none
 * ********************************************/
void checkIn()
{
    char tmp[3];
    int i = 0;
    printf("Enter ID of the book beeing returned: ");

    fgets(tmp, 4, stdin);

    if ((strlen(tmp)>0) && (tmp[strlen (tmp) - 1] == '\n'))
        tmp[strlen (tmp) - 1] = '\0';

    fflush(stdin);

    for (i = 0; i < MAX_BOOKS;i++)
    {
        if(books[i].ID == atoi(tmp))
        {
            if (books[i].isIn == 1)
            {
                printf("Book allready checked here!\n");
                system("pause");
            }
            else
            {
                books[i].isIn = 1;
            }
            break;
        }

    }
}


/*********************************************
 * Function: sort()
 *
 * Description: sorts the Array of books by ID
 * Algorythmus: Bubblsort
 * Parameters: none
 *
 * Return parameters: none
 * ********************************************/

    void sort()
    {
        /* first find out how many indexes there are */
        int h;
        for (h = 0; h< MAX_BOOKS; h++)
        {
            if (books[h].ID == 0)
            {
                break;
            }
        }

        int j =  0;

        int swaped = 1;
        struct book temp;
        while (swaped == 1)       //bubble sort on the book name
        {
            swaped = 0;
            for(j=0;j< h -1 ;j++)
            {

                if(books[j].ID > books[j + 1].ID)
                {

                    //copy to temp val
                    temp.ID = books[j].ID;
                    strcpy(temp.name,books[j].name);
                    strcpy(temp.dateIn,books[j].dateIn);
                    strcpy(temp.dateOut,books[j].dateOut);
                    temp.isIn = books[j].isIn;


                    //copy next val
                    books[j].ID = books[j + 1].ID;
                    strcpy(books[j].name,books[j + 1].name);
                    strcpy(books[j].dateIn,books[j + 1].dateIn);
                    strcpy(books[j].dateOut,books[j + 1].dateOut);
                    books[j].isIn = books[j + 1].isIn;


                    //copy back temp val
                    books[j + 1].ID = temp.ID;
                    strcpy(books[j+ 1].name,temp.name);
                    strcpy(books[j + 1].dateIn,temp.dateIn);
                    strcpy(books[j + 1].dateOut,temp.dateOut);
                    books[j + 1].isIn = temp.isIn;


                    swaped = 1;

                }
            }
        }

    }

void mutateBook(book b)
{
    printf("What do you want to change?\n");

}
\$\endgroup\$
3
\$\begingroup\$
#define MAX_STR_LEN 256
#define MAX_BOOKS 10
#define DATE_LEN 8

How do you choose these numbers? Does you code ensure that data in the file doesn't exceed these numbers?

In fact your readBookFile function has a bug: if the title of a book is nearly MAX_STR_LEN then the length of the whole file-line is greater than MAX_STR_LEN and won't fit into buffer.


/* array of my books */

See @Jackson's answer.


/* PROTOTYPE OF FUNCTIONS */

If you define main at the end of the file, then you may not need these functional declarations because instead you'll have the function definitions already defined, above main, before they're used from main.


 * Description: What is there to explain..
 *
 * Parameters:  argv (the string of all parameters),
 *              argc (the count of all input aruments)

Too many useless comments?


isOK = readBookFile();

You don't test isOK or do anything to recover or exit gracefully (for example, print an error message and return early before trying to write), if readBookFile fails.

In general, assigning to a variable and then not reading/using the value in the variable is suspicious: if you don't read/use it, why assign/remember it in the first place?


system( "read -n 1 -s -p \"Press any key to continue...\"" );

I don't know the system function. Why not printf instead?


FILE *bookFile;

I like to delay my variable definition until the later line of code (immediately before the if ( ( bookFile statement) when I can actually assign a value to it. In general, any uninitialized variable is dangerous.


printf ("No memory\n");

You use whitespace inconsistently: normal style is no whitespace between the function name and its open parenthesis.


while (fgets(buf, 255, bookFile) != NULL)

I think you meant MAX_STR_LEN not 255. What if you change the definition of MAX_STR_LEN? You don't want to hunt through the rest of your source code looking for other 255 instances to change.

Also to fix the bug I mentioned earlier, buf and fgets should use MAX_BOOK_LEN not MAX_STR_LEN:

#define MAX_BOOK_LEN sizeof(Book) + 1

This expression for MAX_BOOK_LEN based on sizeof(Book) assumes that Book.title is an array inside Book (which, it is at the moment), not a pointer to a variable/infinite length title allocated via malloc (which, you might change it to sometime in the future).


    /* if we don't chech for newline we skip every second line */
    if ((strlen(buf)>0) && (buf[strlen (buf) - 1] == '\n'))
        buf[strlen (buf) - 1] = '\0';

You're calling strlen(buf) several times. If you want it to be (very slightly) faster then only call it once:

    size_t buflen = strlen(buf);
    if ((buflen > 0) && (buf[buflen - 1] == '\n'))
        buf[buflen - 1] = '\0';

if(ferror(bookFile))

This isn't necessary according to my reading of some documentation: because if there is any error then fgets would return null.

You might like to call it before you return, so that you can use its return code to say whether there was an error reading from the input file:

/* check for error */
int returncode = ferror(bookFile);

/* free the buffer */
free(buf);

/* close the filestream */
fclose(bookFile);

return returncode; /* 0 if all reads were successful */

/* the getNextID returns the next free ID number */
newID = getNextID();

books[newID].ID= newID + 1;

This was confusing: I wondered, why do you increment the next ID before using it? Don't you want to increment the current ID, or not increment to next ID?

To avoid this confusion, perhaps rename the getNextID() function to getEmptyIndex(), and add a comment to your newID = getEmptyIndex() + 1; statement to say that indexes are 0-based whereas IDs are 1-based.


printf("datein (DDMMYY):");

Perhaps you should verify the format and length of what was entered, and ask the user to retry or quit if it's the wrong syntax.


There are several places, i.e. in addBook and in sort, where you use a series of statements like:

books[newID].ID= newID + 1;
strcpy(books[newID].name, bin.name);
strcpy(books[newID].dateIn, bin.dateIn);
strcpy(books[newID].dateOut, bin.dateOut);
books[newID].isIn =  bin.isIn;

What happens if you add a new field to Book (for example, author) in the future? You'd have to change these statements in several places. Better to have a new common subroutine,

void memcpyBook(Book* toBook, const Book* fromBook) { ... }

I'm worried about linking ID to index. If you do the following:

  1. Create or read several books
  2. Delete some books from the middle of the array
  3. Sort the books

Then the IDs no longer match the array indexes. If you then add another book, you'll find the empty index and maybe create a duplicate Book ID.

\$\endgroup\$
4
\$\begingroup\$

You don't need the two definitions of book:

typedef struct {..} book ;
struct book {..} ;

What I would normally do is:

typedef struct Book {
...
} Book ;

This allows you to write either:

struct Book b1 ;
Book b1 ;

and in both cases get a Book structure. it also means i can refer to a Book structure within a Book structure:

typedef struct Book {
   ...
   struct Book* prequel ;
} Book ;

I'd change the name of the structure and typedef to Book as most coding standards call for structs and typedefs to have an initial capital letter.

In a lot of places you'll see the typedef and the structure given different names,

typedef struct Book_ {...} Book_T ;

it's up to you to decide what you find easiest to use; I said I like to use the same name for both.

Another thing to look at is the readBookFile function, from reading the code I think you need to look at the following:

You use malloc() to allocate the buffer that the file is read into; however you never free the memmory that is allocated by malloc. It's good practice to make sure that every malloc ends up being freed in every route through the function.

You don't check the return value of malloc; in a program like yours it's very unlikely that the malloc call will fail; however you should check the return value.

Do you even need to use malloc here? If you allocate the buffer on the stack then you don't need to worry about freeing it.

The call to fgets() has the magic number 255 in it, you can and should just use the MAX_STR_LEN #define that you created and used in the malloc. Read the man page for fgets(), it tells you that the function already knows that the last char is resevered for a terminating NULL.

The strtok() function call returns NULL if there are no more tokens, again this is something you should check.

You have virtually identical code repeated several times when you call strtok() for each field, you should consider extracting this out into it's own function.

\$\endgroup\$
  • \$\begingroup\$ This is so cool.. I've been asking myself why it didn't work before I had both decinitions. Now I know :-) \$\endgroup\$ – Hagbart Celine Jan 22 '14 at 17:19

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.