views:

95

answers:

5

Hi all. I'm making a small console-based rpg, to brush up on my programming skills. I am using structures to store character data. Things like their HP, Strength, perhaps Inventory down the road. One of the key things I need to be able to do is load and save characters. Which means reading and saving structures.

Right now I'm just saving and loading a structure with first name and last name, and attempting to read it properly.

Here is my code for creating a character:

void createCharacter()
{
    char namebuf[20];

    printf("First Name:");

    if (NULL != fgets(namebuf, 20, stdin))
    {
        char *nlptr = strchr(namebuf, '\n');
        if (nlptr) *nlptr = '\0';
    }
    strcpy(party[nMember].fname,namebuf);


    printf("Last Name:");
    if (NULL != fgets(namebuf, 20, stdin))
    {
        char *nlptr = strchr(namebuf, '\n');
        if (nlptr) *nlptr = '\0';
    }
    strcpy(party[nMember].lname,namebuf);

    /*Character created, now save */
    saveCharacter(party[nMember]);
    printf("\n\n");
    loadCharacter();

}

And here is the saveCharacter function:

void saveCharacter(character party)
{
    FILE *fp;
    fp = fopen("data","a");
    fwrite(&party,sizeof(party),1,fp);
    fclose(fp);

}

and the loadCharacter function

void loadCharacter()
{
    FILE *fp;

    character tempParty[50];
    int loop = 0;
    int count = 1;
    int read = 2;

    fp= fopen("data","r");

    while(read != 0)
    {
        read=fread(&tempParty[loop],sizeof(tempParty[loop]),1,fp);
        printf("%d. %s %s\n",count,tempParty[loop].fname,tempParty[loop].lname);
        loop++;
        count++;
    }
    fclose(fp);
}

So the expected result of the program is that I input a name and last name such as 'John Doe', and it gets appended to the data file. Then it is read in, maybe something like

1. Jane Doe
2. John Doe

and the program ends.

However, my output seems to add one more blank structure to the end.

1. Jane Doe
2. John Doe
3. 

I'd like to know why this is. Keep in mind I'm reading the file until fread returns a 0 to signify it's hit the EOF.

Thanks :)

+1  A: 

Here:

    read=fread(&tempParty[loop],sizeof(tempParty[loop]),1,fp);
    printf("%d. %s %s\n",count,tempParty[loop].fname,tempParty[loop].lname);

You are not checking whether the read was successful (the return value of fread()).

Juliano
+3  A: 

Change your loop:

while( fread(&tempParty[loop],sizeof(tempParty[loop]),1,fp) )
{
    // other stuff
}

Whenever you write file reading code ask yourself this question - "what happens if I read an empty file?"

anon
Thanks, and this goes to all the answers. I understand my mistake now, and since your solution I find to be the most elegant, and was the easiest to implement, I'll make yours the accepted answer.
Blackbinary
+2  A: 

You have an algorithmic problem in your loop, change it to:

 read=fread(&tempParty[loop],sizeof(tempParty[loop]),1,fp);
 while(read != 0)
 {
        //read=fread(&tempParty[loop],sizeof(tempParty[loop]),1,fp);
        printf("%d. %s %s\n",count,tempParty[loop].fname,tempParty[loop].lname);
        loop++;
        count++;
        read=fread(&tempParty[loop],sizeof(tempParty[loop]),1,fp);
 }

There are ways to ged rid of the double fread but first get it working and make sure you understand the flow.

Henk Holterman
A: 

You've got the answer to your immediate question but it's worth pointing out that blindly writing and reading whole structures is not a good plan.

Structure layouts can and do change depending on the compiler you use, the version of that compiler and even with the exact compiler flags used. Any change here will break your ability to read files saved with a different version.

If you have ambitions of supporting multiple platforms issues like endianness also come into play.

And then there's what happens if you add elements to your structure in later versions ...

For robustness you need to think about defining your file format independently of your code and having your save and load functions handle serialising and de-serialising to and from this format.

Nigel Harper
I agree, however this is nothing serious. As i mentioned, its really just practice to brush up on my programming skills before classes start again.
Blackbinary
+1  A: 
while( 1==fread(&tempParty[loop],sizeof*tempParty,1,fp) )
{
/* do anything */
}

is the correct way.

use fopen("data","rb") instead of fopen("data","r") which is equivalent to fopen("data","rt")