Unexpected result when saving struct members to .txt file in C

Question

I am having a few issues while trying to save information from my program to a .txt file (or any file really). I have been looking through my code a few times and I can't seem to find the issue. Initially I was thinking that there might be some form of memory leak (although I do not know the consequences of memory leaks so I can't be sure).

I want to clarify that this is a school assignment and I am in this to learn after all so don't give me the answer too easily!

The assignment is our last and our biggest. We are creating a grocery list with structs. The problem started when I was trying to use the struct members to save information into a .txt file (to load them into the program at a later time if desired). I know the code is probably horrible and eye-hurting to watch but please bear with me.

This is my "save" function. It is very basic and quite horrible.

void saveList(struct GList *grocery)
{
    char file[20];
    FILE *fp;

    printf("What do you want to save the list as? (Don't include file extension): ");
    scanf("%s", file);

    fp = fopen(strcat(file, ".txt"), "w");

    for (int i=0; i<grocery->Items; i++)
    {
        printf("%s %f %s\n", grocery->list[i].name, grocery->list[i].amount, grocery->list[i].unit);
        fprintf(fp, "%s %f %s\n", grocery->list[i].name, grocery->list[i].amount, grocery->list[i].unit);
    }
    fclose(fp);
}

This is what I input in my program (when adding an item):

Name of the item: Avocado
Unit: kg
Amount: 10

This is what is getting saved to my .txt file (it's not showing, but the first line always contains some weird symbol).

  10.000000 kg
milk 10.000000 litres

The same problem occurs all the time; the first item name (Avocado e.g.) displays as some weird symbol.

This is my full code, the problem might be lying somewhere in here.

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

struct Grocery {
    char name[20];
    char unit[20];
    float amount;
};

struct GList {
    size_t Items;
    struct Grocery *list;
};

int addGrocery();
void printList();
void hQuit();
void inputError();
void removeItem();
void changeItem();
void saveList();

int main()
{
    struct GList glist;
    glist.Items = 1;

    size_t menuChoice = 0;
    char cont = 'y';

    if((glist.list = malloc(sizeof(glist.list))) == NULL)
        return ENOMEM;

    puts("Welcome to your Grocery List Manager");

    do
    {
        printf("\n- - - - - - - - - - -\n[1] Add an item\n[2] Print grocery list\n[3] Remove a grocery\n[4] Edit a grocery\n[5] Save your list\n[6] Load a list\n[7] Quit\n\nPlease choose action: ");
        if(!scanf("%u", &menuChoice))
            return EIO;

        putchar('\n');

        switch(menuChoice)
        {
            case 1:
                addGrocery(&glist);
                break;
            case 2:
                printList(&glist);
                break;
            case 3:
                removeItem(&glist);
                break;
            case 4:
                changeItem(&glist);
                break;
            case 5:
                saveList(&glist);
                break;
            case 6:
                //Load shopping list
                break;
            case 7:
                hQuit(&glist);
                break;
            default:
                inputError();
                break;
        }
    } while (cont == 'y');

    //free(grocery);

    return 0;
}

int addGrocery(struct GList *grocery)
{
    printf("Name of the grocery: ");
    if(!scanf("%s", grocery->list[grocery->Items].name))
        return EIO;

    printf("Unit: ");
    if(!scanf("%s", grocery->list[grocery->Items].unit))
        return EIO;

    printf("Amount: ");
    if(!scanf("%f", &grocery->list[grocery->Items].amount))
        return EIO;

    printf("You have added %f %s of %s into your list!\n\n", grocery->list[grocery->Items].amount, grocery->list[grocery->Items].unit, grocery->list[grocery->Items].name);

    (grocery->Items)++;
    grocery->list = realloc(grocery->list, grocery->Items * sizeof(grocery->list));

    if(grocery->list == NULL)
        return ENOMEM;

    return 1;
}

void printList(struct GList *grocery)
{
    if ((grocery->Items - 1) > 0)
        printf("You have added %d item(s) into your list!\n", grocery->Items - 1);
    else
        printf("You have no items in your list!\n");

    for (int i=1; i<grocery->Items; i++)
    {
        printf("[%d] %-10s %.1f %s\n", i, grocery->list[i].name, grocery->list[i].amount, grocery->list[i].unit);
    }
    putchar('\n');
}

void removeItem(struct GList *grocery)
{
    size_t index = 0;
    printf("Which item would you wish to remove from the list? ");
    scanf("%u", &index);
    printf("\nYou have removed %s from your grocery list!", grocery->list[index].name);
    for (int i=(int)index; i < grocery->Items; i++)
        grocery->list[i] = grocery->list[i+1];

    (grocery->Items)--;
}

void changeItem(struct GList *grocery)
{
    size_t index = 0;
    printf("Which item would you like to edit the amount of? ");
    scanf("%d", &index);
    printf("\nCurrent amount: %.1f %s\nEnter new amount: ", grocery->list[index].amount, grocery->list[index].unit);
    scanf("%f", &grocery->list[index].amount);
    printf("\nYou changed the amount to %.1f!\n", grocery->list[index].amount);
}

void hQuit(struct GList *grocery)
{
    puts("*-*-* Thank you for using the Grocery List! *-*-*");
    free(grocery->list);
    exit(0);
}

void inputError(struct GList *grocery)
{
    puts("No such option. Please try again!\n");
}

void saveList(struct GList *grocery)
{
    char file[20];
    FILE *fp;
    printf("What do you want to save the list as? (Don't include file extension): ");
    scanf("%s", file);
    fp = fopen(strcat(file, ".txt"), "w");
    for (int i=0; i<grocery->Items; i++)
    {
        printf("%s %f %s\n", grocery->list[i].name, grocery->list[i].amount, grocery->list[i].unit);
        fprintf(fp, "%s %f %s\n", grocery->list[i].name, grocery->list[i].amount, grocery->list[i].unit);
    }

    fclose(fp);
}

If my code looks very peculiar in some places (why do you have a void inputError()?) it's because our teacher sets some very odd rules for our assignments.

Please feel free to bash my code.

Answer 1

For a start, I'm sure your teacher will tell you many more times not to use magic numbers:

char file[PATH_MAX];

---

You would probably want to avoid overflowing this buffer, for the sake of your programs future computational rationality:

if (snprintf(NULL, 0, "%s.txt", file) >= PATH_MAX - 1) {
    fputs("Filename too long!", stderr);
    exit(EXIT_FAILURE);
}

---

if (scanf("%s", grocery->list[grocery->Items].name) == 1)

You would not know that you're using scanf incorrectly until you read the scanf manual (which is part of our jobs as software developers). In fact, it may not even seem obvious from a cursory glance that you're doing something wrong.

Indeed, as software developers not only must we carefully read manuals, error messages, code written by other people (which might not reflect poor-quality comments very well).

Checking if scanf returns 0 is a good way to determine if 0 elements were read, but not a good way to determine if EOF or some other file access error occurred.

Can you work out why I (correctly) compared against 1 instead? If you wanted to read two values from stdin using a single comparison to scanf, which numeric value should you compare the return value to?

---

void *temp = realloc(grocery->list, grocery->Items * sizeof *grocery->list);
if (temp == NULL)
    return ENOMEM;
grocery->list = temp;

You would not know that you're using realloc incorre... read the realloc manual yada ... yada ... etc so on and so forth

There is another modification I made here: you missed an asterisk! D'oh! See if you can spot it :)

Indeed, as software developers not only must we carefully read manuals, error messages, code written by other people (which might not reflect poor-quality comments very well).

As a result, we must make some deductions from the manual to determine when realloc might not free the old pointer before you overwrite it (thus causing a memory leak).

Can you work out why I used a temporary variable, there (as you should be)?

Similarly, you missed another asterisk here:

if((glist.list = malloc(sizeof glist.list[0])) == NULL)

Sorry, I couldn't help but make it a bit more obvious... You should make a mental note to follow these patterns:

pointer = malloc(count * sizeof *pointer);               // -- the `malloc` pattern; pointer could be NULL, so needs checking
void *temp = realloc(pointer, count * sizeof *pointer);  // -- the `realloc` pattern; temp could be NULL (in which case you need to deal with `pointer`)

Remember those two patterns and you shouldn't find yourself making these mistakes again.

While we're on the topic of lists, empty ones contain 0 items, right?

glist.Items = 0;

---

P.S. Have you ever heard of valgrind?...

Answer 2

Ask yourself, "does C use 0-based or 1-based array indexing"?

When you call addGrocery, you pass the address of glist,

addGrocery(&glist);

What is the first/initial value of glist when you call addGrocery the first time? How many items does this list contain, before you add the first item? Is this a "list", or an "array"?

Here are the first few lines of your main function (which answers that question),

int main()
{
    struct GList glist;
    glist.Items = 1;

    if((glist.list = malloc(sizeof(glist.list))) == NULL)
        return ENOMEM;

Consider defining a function (a constructor) to create the initial (empty) list. And a function to add an element to the list.

Your addGrocery function conflates entering data and adding the data to the list. Consider a function that merely collects input then calls the function to add the data to the list.

int addGrocery(struct GList *grocery)
{
    printf("Name of the grocery: ");
    //what is the value of grocery-Items the first time this is called?
    if(!scanf("%s", grocery->list[grocery->Items].name))
        return EIO;

    //Consider something that creates a grocery list item (does malloc)
    //then appends that list item to the list

    //then this check would not be needed (well, it would change)
    if(grocery->list == NULL)
        return ENOMEM;

Hint: Are you adding to the first list element?

But there is an even larger problem. Are you using an array or a list to store struct Grocery items? You declare list as a pointer, and you initialize it in main. Have you allocated an array of some number of items, or do you want a list of items? The struct Grocery type has no pointers, so you probably don't want a "list", but rather an "array" (naming is important).

struct GList {
    size_t Items;
    struct Grocery *list;
};

Because your addGrocery function uses array indexing, assume you want an array of Grocery items, but how many have you created? And which one are you addressing?

(these questions should point you in the right direction)