1

I have a function that is supposed to read through a file, put the individual lines, as separate elements, into an array. Then it's supposed to go through the array and put certain elements at certain positions within the struct.

I almost have it... When I go to print the struct to make sure it's getting everything right, extra characters are showing up!

This is what is in the file:

123
pre
45
cse
67
345
ret
45
cse
56

And this is what it's printing:

123
pre
45
cse
C
67
345
ret
45
cse
8
56

Here is the code:

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

struct students         //Defining structure for students
{
    int id;        //Students ID Number
    char name[30];      //Students Name
    int age;            //Students Age
    char dept[4];       //Studets Department
    int grade;          //Students Grade
};



int main()
{
    struct students list[20];
    FILE *f;
    char line[30];
    char **temp = NULL;
    int num_righ = 0;
    int id5;
    int age5;
    int grade5;
    int i, k;

    f = fopen("records.txt", "r");



    while(fgets(line, sizeof (line), f) != NULL)
    {
        if (line != NULL)
        {
            num_righ++;
            temp = (char**)realloc(temp, sizeof(char*) *num_righ);
            temp[num_righ - 1] = strdup(line);
        }
    }

    fclose(f);
    k = 0;
    i = 0;
    while (temp[i] != NULL)
    {
        id5 = atoi(temp[i]);
        list[k].id = id5;
        i++;
        strcpy(list[k].name, temp[i]);
        i++;
        age5 = atoi(temp[i]);
        list[k].age = age5;
        i++;
        strcpy(list[k].dept, temp[i]);
        i++;
        grade5 = atoi(temp[i]);
        list[k].grade = grade5;
        i++;
        k++;


    }
    for (i = 0; i < k; i++)
    {
        printf("%d\n", list[i].id);
        printf("%s", list[i].name);
        printf("%d\n", list[i].age);
        printf("%s\n", list[i].dept);
        printf("%d\n", list[i].grade);
    }
}
Joe DF
  • 5,438
  • 6
  • 41
  • 63
P M
  • 195
  • 1
  • 12
  • 3
    Why not process each line as you read it, instead of copying it to a temp array? Your method takes twice as much memory with no obvious benefit... – Floris Oct 31 '13 at 01:33
  • 1
    `strcpy()` doesn't check if there is room. Use `strcpyn()` – mike jones Oct 31 '13 at 01:47
  • @Floris I have thought about how to do that and this was the best solution I could come up with (I'm still a newbie). How would you do it? Each line is a new element in the struct and after 5 lines it starts a new element within the array struct. – P M Oct 31 '13 at 01:50
  • 1
    I have given an example of how to do that in an answer (below). – Floris Oct 31 '13 at 02:16

2 Answers2

4

One thing to note is that the decimal value for 'C' is 67, and the decimal value for '8' is 56. Your dept array in your students array is too small. It is grabbing the newline character, and is then unable to store a terminating character. The printf runs through to the grade integer, which is printed as a char.

EDIT: Rather, your array isn't too small, but fgets is grabbing the newline, which fills the array, preventing the null terminator from being stored properly.

Taywee
  • 1,313
  • 11
  • 17
  • 1
    Not necessarily. A cleaner way is to remove newlines from your input, which is usually what's wanted anyway. http://stackoverflow.com/questions/2693776/removing-trailing-newline-character-from-fgets-input – Taywee Oct 31 '13 at 01:36
  • 2
    To clarify this answer: `fgets` takes the line plus the new line character plus a terminating `'\0'`, which is a total of five characters. Your `dept` is an array of four elements. Make it one bigger in your `struct` definition and all will be well. Or strip the new line character before the copy. – Floris Oct 31 '13 at 01:38
  • 1
    Oh, you'll notice that when you are printing your `name` from the struct, you don't put your own newline in, it's using the one extracted from the file. It's also worth noting that if you were running on a big-endian architecture, you wouldn't have seen the problem this way, because your print routine would have hit the 0 bytes of the integer before the non-zero ones, and would see the string as terminated there. Here it happens after printing the least significant byte as a char. – Taywee Oct 31 '13 at 01:39
  • @Taywee I noticed that when I've run it with it in there before. Also, if I don't remove the newline character, and I'm taking these same elements (that have the newline character) from the file, adding more elements within the struct, then printing all the elements of the struct to the file, and keep repeating this, are the newline characters going to build up? Probably a stupid question, but I'm curious. – P M Oct 31 '13 at 01:47
  • More likely, you'll end up with blank lines in the file that the program attempts to read as members, which will cause failure in other fun and exciting ways, unless you pay very close attention to where you expect a newline to be, especially when normally using the members in the program. `fgets` stops after the first newline it sees, so if it is used to read a blank line, it will only put a `\n\0` into the array. – Taywee Oct 31 '13 at 01:50
1

The following code addresses multiple problems - not only does it make sure that strings are "safely" copied (using strncpy, and terminating the string with a '\0'), but it also makes sure you don't create a second copy of all the data in memory (not a problem with a toy example, but why start with bad habits).

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

struct students         //Defining structure for students
{
    int id;        //Students ID Number
    char name[30];      //Students Name
    int age;            //Students Age
    char dept[4];       //Studets Department
    int grade;          //Students Grade
};

int main()
{
    struct students list[20];
    FILE *f;
    char line[30];
    char **temp = NULL;
    int num_righ = 0;
    int id5;
    int age5;
    int grade5;
    int i, k=0;
    char *newLine;

    f = fopen("records.txt", "r");
    int s = 0;  // this is the "state" counter - it goes from 0 to 4, then back to 0

    while(fgets(line, sizeof (line), f) != NULL)
    {
       newLine = strchr(line, '\n');
       if(newLine) *newLine='\0'; // terminate string on the newline.
        switch(s) {
          case 0:
            list[k].id = atoi(line);
            break;
          case 1:
             strncpy(list[k].name, line, 30);
             list[k].name[29]='\0'; // make sure it is terminated
             break;
          case 2:
            list[k].age = atoi(line);
            break;
          case 3:
            strncpy(list[k].dept, line, 3);
            list[k].dept[3] = '\0'; // make sure it is terminated
            break;
          case 4:
            list[k].grade = atoi(line);
            break;
        }
        s++;
        if (s == 5) {
          s = 0;
          k++; // if it's 5, go back to zero and start reading next structure
        }
      }
    fclose(f);

    for (i = 0; i < k; i++)
    {
        printf("id: %d\n", list[i].id);
        printf("name: %s", list[i].name);
        printf("age: %d\n", list[i].age);
        printf("dept: %s\n", list[i].dept);
        printf("grade: %d\n\n", list[i].grade);
    }
}
Floris
  • 45,857
  • 6
  • 70
  • 122
  • Wow, that just blew my mind on how simple that was... haha thanks for showing me! :) – P M Oct 31 '13 at 02:22
  • Also, I noticed when you strip the newline character, you just had it strip the very last element; is that where the new line will always be, even if the string isn't full? – P M Oct 31 '13 at 02:29
  • 1
    You're welcome! You can "upvote" or "accept" answers you like, now that you have 15 points of reputation... – Floris Oct 31 '13 at 02:30
  • 1
    No - what I did was make sure that if the input line was longer than the variable I am copying into, there is still going to be a terminating nul character. It would be better to strip the new line from `line` before copying... – Floris Oct 31 '13 at 02:31
  • 1
    See updated code that deals with newLine properly, and still leaves the "defensive nul termination" in place. – Floris Oct 31 '13 at 02:39