10
\$\begingroup\$

I'm currently writing a menu driven program to store the data for a collection of books. So far the program displays a menu, allows the user to add a book, view the list of books, and exit the program.

Note that the program allows the user to cancel a book entry by pressing a lowercase 'c' and then the ENTER key. The program also restricts the number of characters a user is allowed to enter to 50 characters.

Questions

  1. Is there a better approach in allowing a user to cancel a book entry?

  2. Are there any concerns or issues that may arise and pose a potential problem?

main.c

/* program to maintain a database for a collection of books */

#include "myheader.h"
#include "database.h"
#include "userinput.h"

/* main: display menu options */
int main(int argc, char *argv[])
{
    const char fname[] = "database.bin";
    char userInput[MAXLEN + 1];
    int sel;

    while (1)
    {
        system("clear");
        printf("                                                                             \n");
        printf("=============================================================================\n");
        printf("                 Database for Book Collection                                \n");
        printf("                        Menu Options                                         \n");
        printf("=============================================================================\n");
        printf("                                                                             \n");
        printf("  1. Add record                                                              \n");
        printf("  2. Delete record                                                           \n");
        printf("  3. Modify record                                                           \n");
        printf("  4. View record                                                             \n");
        printf("  5. View all                                                                \n");
        printf("  6. Exit                                                                    \n");
        printf("                                                                             \n");
        printf("=============================================================================\n");
        printf("                                                                             \n");
        getUserInput("Choose menu option: ", userInput, MAXLEN+1);
        sel = atoi(userInput);
        switch (sel)
        {
            case 1:
                addRecord(fname);
                break;
            case 5:
                viewAll(fname);
                break;
            case 6:
                system("clear");
                exit(0);
            default:
                printf("Invalid menu option\n");
                printf("Press ENTER to continue\n");
                flushInput();
                break;
        }
    }

    return 0;
}

myheader.h

#ifndef __MYHEADER_H__
    #define __MYHEADER_H__

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

    #define MAXLEN 50
#endif

database.h

#ifndef __DATABASE_H__  
    #define __DATABASE_H__
    typedef struct book
    {
        char title[MAXLEN + 1];
        char author[MAXLEN + 1];
        double price;
    } book;
    void addRecord(const char []);
    void viewAll(const char[]);
#endif

database.c

#include "myheader.h"
#include "database.h"
#include "userinput.h"

/* addRecord: add record to database */
void addRecord(const char *fname)
{
    FILE *fp;
    book bk;
    char userInput[MAXLEN + 1];
    double price;
    int saveRecord;

    while(1)
    {
        system("clear");
        printf("                                                                             \n");
        printf("=============================================================================\n");
        printf("                 Database for Book Collection                                \n");
        printf("                         Add Record                                          \n");
        printf("=============================================================================\n");
        printf("                                                                             \n");

        /* prompt user for title */
        while (1)
        {
            getUserInput("Title ['c' to cancel]: ", userInput, MAXLEN + 1);
            if (userInput[0] == '\0')
                printf("Title is missing\n");
            else if (strcmp(userInput, "c") == 0)
                return;
            else
            {
                strcpy(bk.title, userInput);
                break;
            }
        }

        /* prompt user for author */
        while (1)
        {
            getUserInput("Author ['c' to cancel]: ", userInput, MAXLEN + 1);
            if (userInput[0] == '\0')
                printf("Author is missing\n");
            else if (strcmp(userInput, "c") == 0)
                return;
            else
            {
                strcpy(bk.author, userInput);
                break;
            }
        }

        /* prompt user for price */
        if (!getPrice(&price))
            return;
        else
            bk.price = price;

        /* prompt user to save record */
        while (1)
        {
            getUserInput("Save record? [Y/n] ", userInput, MAXLEN + 1);
            if (strcasecmp(userInput, "Y") == 0)
            {
                saveRecord = 1;
                break;
            }
            else if (strcasecmp(userInput, "N") == 0)
            {
                saveRecord = 0;
                break;
            }
        }

        /* save record if user answered yes when prompted to save */
        if (saveRecord)
        {
            if ((fp = fopen(fname, "ab")) == NULL)
            {
                printf("addRecord(): unable to open %s\n", fname);
                exit(1);
            }
            if (fwrite(&bk, sizeof(bk), 1, fp) != 1)
            {
                printf("addRecord(): unable to write to %s\n", fname);
                fclose(fp);
                exit(1);
            }
            fclose(fp);
        }

        /* prompt user to add another record */
        while (1)
        {
            getUserInput("Add another record? [Y/n] ", userInput, MAXLEN + 1);
            if (strcasecmp(userInput, "Y") == 0)
                break;
            else if (strcasecmp(userInput, "N") == 0)
                return;
        }
    }
}

/* viewAll:  display the details for all records in the database */
void viewAll(const char fname[])
{
    FILE *fp;
    book *bks;
    int i, numbks;

    if ((fp = fopen(fname, "rb")) == NULL)
    {
        printf("viewAll(): unable to open %s\n", fname);
        exit(1);
    }

    /* calculate the number of books stored in file */
    fseek(fp, 0, SEEK_END);
    numbks = ftell(fp) / sizeof(book);
    fseek(fp, 0, SEEK_SET);

    /* allocate memory to store books from file */
    if ((bks = (book *)malloc(numbks * sizeof(book))) == NULL)
    {
        printf("viewAll(): unable to malloc bks\n");
        fclose(fp);
        exit(1);
    }

    /* read all books from file */
    if (fread(bks, sizeof(book), numbks, fp) != numbks)
    {
        printf("viewAll(): unable to read from %s\n", fname);
        free(bks);
        fclose(fp);
        exit(1);
    }

    system("clear");
    printf("                                                                             \n");
    printf("=============================================================================\n");
    printf("                 Database for Book Collection                                \n");
    printf("                         View All                                            \n");
    printf("=============================================================================\n");

    for (i = 0; i < numbks; i++)
    {
        printf("\nTitle: %s\nAuthor: %s\nPrice: $%.2f\n", bks[i].title, bks[i].author, bks[i].price);
    }

    printf("                                                                             \n");
    printf("=============================================================================\n");

    free(bks);
    fclose(fp);

    printf("\nPress ENTER to continue\n");
    flushInput();
}

userinput.h

#ifndef __USERINPUT_H__
    #define __USERINPUT_H__
    void getUserInput(const char [], char [], const int);
    int getPrice(double *);
    void flushInput();
#endif

userinput.c

#include "myheader.h"
#include "userinput.h"

/* getUserInput: get response from user based on prompt */
void getUserInput(const char prompt[], char s[], const int size)
{
    int c, i;

    printf("%s", prompt);

    i = 0;
    while ((c = getchar()) != '\n' && c != EOF)
    {
        s[i++] = (char)c;
        if (i > size - 1)
        {
            s[0] = '\0';
            i = 0;
            flushInput();
            printf("\nMaximum of %d characters exceeded\n", size - 1);
            printf("%s", prompt);
        }
   }

   s[i] = '\0';
}

/* getPrice: prompt user for price */
int getPrice(double *price)
{
    char userInput[MAXLEN+1], junk[MAXLEN+1];

    while (1)
    {
        getUserInput("Price ['c' to cancel]: ", userInput, MAXLEN+1);
        if (userInput[0] == '\0')
            printf("Price is missing\n");
        else if (strcmp(userInput, "c") == 0)
            return 0;
        else if (sscanf(userInput, "%lf%[^\n]", price, junk) == 1)
            return 1;
    }
}

void flushInput()
{
    int c;

    while ((c = getchar()) != '\n' && c != EOF)
        /* skip it */ ;
}

Compiled as follows:

gcc main.c database.c userinput.c -o database
\$\endgroup\$
7
\$\begingroup\$

I see some things that may help you improve your code.

Use all required #includes

The code uses strcasecmp() which is defined in <strings.h> on my machine (and any POSIX-conforming machine, I suspect) but only <string.h> (also needed) is included.

Don't use system("clear")

One problem with system("clear") is that it's not portable to other operating systems which you may or may not care about now. Another reason is that it might not be good for performance because system creates a fork which is not a lightweight operation. If your terminal supports ANSI Escape sequences, you could instead use this:

void clearScreen()
{
    printf("\x1b[2J");
}

It isolates the code to one function, enhancing portability, and it does not spawn a new shell. Note also that this works under Linux, Windows and, I believe, Mac. If you want to also have the cursor set to the upper left hand corner, use this:

printf("\x1b[2J\x1b[1;1H");

Think of the user

It's annoying to the user to be presented a menu of 6 choices, only to have half of them return "Invalid menu option" if selected! It might also be nice to allow the user to specify the database file location. I understand that you're probably going to add those later, but that leads me to the next suggestion.

Think about security

The memory used for each book record is never explicitly set before writing to disk. This could be a security flaw as whatever memory happened to have been allocated might contain sensitive data. To understand this more thoroughly, here's the line in database.c:

book bk;

That's a declaration but not initialization. Because it's an automatic variable, here's what the C11 Standard section 6.7.9 says:

If an object that has automatic storage duration is not initialized explicitly, its value is indeterminate.

I'd recommend declaring and initializing like this:

book bk = {0};

This has the effect of zeroing out all of the structure members.

Use string concatenation

It's really not necessary to make multiple calls to printf to show the menu. You can condense it into a single call without sacrificing readability like this:

printf("                                                                             \n"
       "=============================================================================\n"
       "                 Database for Book Collection                                \n"
       "                        Menu Options                                         \n"
       "=============================================================================\n"
       "                                                                             \n"
       "  1. Add record                                                              \n"
       "  2. Delete record                                                           \n"
       "  3. Modify record                                                           \n"
       "  4. View record                                                             \n"
       "  5. View all                                                                \n"
       "  6. Exit                                                                    \n"
       "                                                                             \n"
       "=============================================================================\n"
       "                                                                             \n");

However, personally, I'd eliminate all of those trailing spaces and save a few hundred bytes of code space.

Make the program more data driven

Much of what the program does is to present a prompt and get an answer. You've started to encapsulate some of that by creating the getUserInput() function, but I think it could be further improved. For example, we could have three variations of that function to return an int, a double or a string. That would eliminate a lot of repeated code. Also, if we consider the idea of a menu, it shows, perhaps a header and a footer and a series of numbered choices. Each of the numbered choices corresponds to some action. One idea would be to create a menu function which takes an array of name, function pairs. The code could display the name, automatically numbering each one, and then execute the passed function pointer when the option is selected.

Consider a different file structure

Right now, the program saves the data as a raw set of memory blocks in the form of book structures. While this is certainly a simple way to do things, there are also some drawbacks. First, it's often nice to have some metadata within the file's header. For example, if this is version one and version two includes a publication year of each book, there's no way for the program to know which version of the database it has opened. I'd suggest creating a file header which could have some useful information in it, such as the version, and perhaps the number of books within the file.

Don't use leading underscores in names

Anything with a leading underscore is a reserved name in C (and in C++). See this question for details.

Don't cast the return from malloc

The return value from malloc or calloc is a void * and does not need an explicit cast. See this question for a thorough discussion of the reasons not to, but for me the most compelling reason is that it's simply not needed.

Use named parameters in header files

The header file is very important because it provides the documentation to other programmers for how to use the code you've written. Consider the database.h file:

#ifndef __DATABASE_H__  
        #define __DATABASE_H__
        typedef struct book
        {
                char title[MAXLEN + 1];
                char author[MAXLEN + 1];
                double price;
        } book;
        void addRecord(const char []);
        void viewAll(const char[]);
#endif

There are a number of problems with this. First, MAXLEN is clearly an important parameter here, but it's missing. The code relies on some other file to define it, and therefore is sensitive to the order of include files. That's fragile code and should be addressed. Second, we can see that addRecord takes a const char [] but what does it mean? I'd have written that function prototype this way instead:

int addRecord(const char *filename);

Now it's clear that what is being passed in is a file name. I've also changed it to return an int which is addressed in the next suggestion.

Separate interface from implementation

The interface is what goes into the .h header file, while the implementation is what goes into the .c source code file. What you want is for the header file to describe everything that a user of the code in the matching .c file would need to know, but not any of the implementation details. So for instance, as mentioned above, MAXLEN is needed by the database.h interface, so it should be in there. I'd recommend eliminating the myheader.h entirely. Instead, each implementation file should include only the pieces each one needs, and the header files should be as independent as possible. Unfortunately, the current implementation needs MAXLEN in every file. This suggests a design flaw. In fact, the only use of MAXLEN in userinput.c is coincidental. All that's really need is some constant that's large enough for the user to type in a price and it isn't necessarily tied to the MAXLEN value that defines the size of the book structure elements. Within main the same is true -- all that's needed is some constant value that is large enough for the user to type in a menu choice.

Return something useful from functions

If the file is empty or doesn't exist, the viewAll() function prints an error message and quits the program. I'd suggest instead that an error value could be returned to the caller and have it decide what to do. That would make it a nicer experience for the user, as well, who might just want to verify that there is no existing database before adding new entries.

Omit return 0

If your program completes successfully, the return 0 at the end of main() will be generated automatically, so it's not needed in C programs. Some people feel strongly that one should write it explicitly anyway. Do what you think is right.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thank you very much for taking the time to review my code. These additional suggestions are extremely helpful. This is great. \$\endgroup\$ – Domenic Apr 24 '18 at 3:55
  • \$\begingroup\$ I've removed strcasecmp, so I won't need to include strings.h. But if the escape sequence needed to clear the screen is depending on the terminal supporting ANSI, wouldn't it be better to use a conditional include based on the operating system, as Coal has described? And since bk is declared as an automatic variable, not a pointer, I'm not sure why we need to explicitly set its memory? Can you please elaborate? \$\endgroup\$ – Domenic Apr 24 '18 at 3:56
  • \$\begingroup\$ With regards to MAXLEN, each source file refers to it. Should I define it in each of their respective header files? Somehow that doesn't seem right. Maybe I'm missing something. Lastly, when I omit return 0 the compiler issues a warning or error, depending on the flag. So if I wanted to omit it (although, I don't think I will), would I just ignore the warning or error? \$\endgroup\$ – Domenic Apr 24 '18 at 4:32
  • \$\begingroup\$ I've expanded my answer to address your questions. If your compiler is emitting a warning when you omit return 0 then it is either a very old compiler (pre C99) or there is a setting to suppress that faulty warning. If you're using gcc try gcc -std=c11 to tell it to use the 2011 version of the standard. There's little point to learning C the way it was done 30 years ago. \$\endgroup\$ – Edward Apr 24 '18 at 9:43
  • 1
    \$\begingroup\$ That's great. Thanks for clarifying. This has been a great learning experience. I really appreciate it. By the way, even with the 2011 version of the standard, the compiler issues a warning when return 0 is omitted. \$\endgroup\$ – Domenic Apr 24 '18 at 14:31
6
\$\begingroup\$

What follows is a list of things I think you could improve on. I am not a C developer, so feel free to correct me if I'm wrong.

  • 'myheader.h' is a pretty generic name for a header. If you can give it a more meaningful name, that would help other developers (or you in 6 months) remember what the purpose of the header is.

  • system("clear"); is a non-portable (and potentially insecure) way of clearing the display. It may work on your system, but what if you need to run this on another platform later? If there's a stable, portable implementation somewhere, it's a good idea to use that instead. For now, you can check if you're on Linux / MacOS, or Windows, to get reasonable portability:

    #if defined(__linux__) || (defined(__APPLE__) && defined(__MACH__))
    void clear() {
        system("clear");
    }
    # else
    void clear() {
        system("cls");
    }
    # endif
    
  • Identifiers starting with double underscores are reserved. According to the C standard, section 7.1.3:

    • All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use.

    So don't use them for your header guards.

  • Error messages should go to stderr. Use fprintf for that (i.e. fprintf(stderr, "My error message");).

  • Always use braces around if-blocks. You wouldn't want something as simple as forgetting to add braces turn into a huge security vulnerability (looking at you, Apple). The same goes for the while-loop in flushInput().

  • Are you trying to comply to C89? Since C99, you don't need to declare variables at the top of blocks. It is preferable to initialize variables when you declare them, and only to declare them once you actually need them.

  • Talking about compliance, compiling with flags like -std=c99 -Wall -Werror -Wextra -pedantic is probably not a bad idea.

  • In addRecord(), the way the program currently asks for a yes/no response is misleading. The prompt suggests the user must enter either 'Y' or 'n'. However, if the user does enter 'n', the same question is asked again (until a literal 'Y' or 'N' is given). If you want this to be case-insensitive, you should allow 'y', 'Y' and 'n', 'N'. Otherwise, you should change the prompt to:

    "Perform action x? [Y/N] "
    

\$\endgroup\$
  • 2
    \$\begingroup\$ First, thank you very much for taking the time to review my code. These are all great suggestions, which I'll be implementing right away. By the way, the yes/no response is actually case-insensitive. But I think you're right, as it stands [Y/n] might be misleading. So I'll change it so that there's no misunderstanding. Thanks again! \$\endgroup\$ – Domenic Apr 23 '18 at 13:11

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.