Thread: While loop skipping first fgets - also reading text file into struct

In your first version, you use fgets followed by sscanf. However, in your second version you start out by doing fgets (which reads a line) and then fscanf (which continues reading AFTER the line you just read).

In my opinion you should use the first approach that you wrote (fgets+sscanf). It means fgets is the only function that is reading from your file handle.

Originally Posted by unsolicited

Thank you. I had considered that...
But the following loop is one of my many attempts at code fiddling that gives me a segmentation fault - and I can't figure out why.

Code:

         sscanf (line, "%s %s %d", card[i].color, card[i].value, &card[i].point_value);

Do your members card[i].color, card[i].value actually have enough room to store the strings? One way to make sure is to make those members at least the same size as line.

Code:

line[strlen (line)-1] = '\0';

If a line read from the file happens to be exactly the size of line (minus 1 for the trailing null), then the newline is not stored and this code will instead overwrite a legitimate character. For example, if your array is 10 characters long and you read in something that is exactly 9 characters (the 10th is the newline), your array at index 8 (index 9 is the null) would be the last character before the newline. This is probably not something you want to overwrite. You want to check first that this character is the newline character before you nullify it. Don't assume it will always be the newline.

Are you sure that the file has the proper number of lines - 108 - and not any more? If this is not the case you currently have no bounds checking on the indexing variable i that would prevent more lines being processed and stored into an array that cannot handle the data. Maybe add an && i < 108 check to the while loop's condition.

The sscanf function returns a value, the number of arguments that were successfully read (should be 3 in your case). Maybe add a check that this value is as expected and print a message if the value is anything other than 3.

Are you certain that none of your strings in the file for color/value are larger than what is stated in your struct, 6 and 14 characters (and + 1 for the null)?

Originally Posted by hk_mp5kpdw

If a line read from the file happens to be exactly the size of line (minus 1 for the trailing null), then the newline is not stored and this code will instead overwrite a legitimate character.

I've set my line to [255].
My longest line in my file is 23 characters.

Originally Posted by c99tutorial

Do your members card[i].color, card[i].value actually have enough room to store the strings? One way to make sure is to make those members at least the same size as line.

Yes.
From my file:
The longest string in "value" is 14 (so in my struct i've set char value[15])
The longest string in "color" is 6 (so in my struct i've set char color[7])
But as per your advice i set each of those to [255] and tried again... no change.

Originally Posted by hk_mp5kpdw

Code:

line[strlen (line)-1] = '\0';

You want to check first that this character is the newline character before you nullify it.

I'm not clear on how to do that check, but I did try blowing away that entire line and it had no effect.

I should make it clear that if I use a "for" loop

Code:

 int i;
    for (i = 0; i < 108; i++) { 
        fgets (line, sizeof(line), f_in);
        line[strlen (line)-1] = '\0';
        sscanf (line, "%s %s %d", card[i].color, card[i].value, &card[i].point_value);
        }

that runs until "i" hits "107" it works no problem. All the values are loaded just fine. String lengths, array sizes... everything works.
I was just trying to do it with a "while" loop that stops at the end of the file in the spirit of making this function less specific to this program.

But if i replace the "for" loop (above) with this "while" loop, it skips the first line.

Code:

int i = 0;
while (fgets (line, sizeof(line), f_in) != NULL) {
        line[strlen (line)-1] = '\0';
        fscanf(f_in, "%s %s %d", card[i].color, card[i].value, &card[i].point_value); 
        i++;
    }

And if I use this "while" loop instead, I get a segmentation fault.

Code:

int i = 0;
while (fgets (line, sizeof(line), f_in) != NULL) {
        line[strlen (line)-1] = '\0';
        sscanf (line, "%s %s %d", card[i].color, card[i].value, &card[i].point_value);
        i++;
    }

Thank you so much for trying to help. i really appreciate it.
Am i posting correctly? Is it better if i post the entire source code? i was trying to keep my post more concise.

Originally Posted by unsolicited

Am i posting correctly? Is it better if i post the entire source code? i was trying to keep my post more concise.

Concise is good. Sometimes the entire source helps. I took your code and ran it and here's what I got after a few small changes. It runs for me without trouble. Notice I added a few checks for errors. Not strictly necessary, but if something goes wrong it helps to know where it happens.

Code:

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

#define error(LINE_NO, LINE_STR) error_("%s:%d: Error occured while processing input line %d:\n%s", \
    __FILE__, __LINE__, (LINE_NO), LINE_STR)

static inline void error_(const char *fmt, const char *file, int line, int line_no, const char *line_str)
{
    fprintf(stderr, fmt, file, line, line_no, line_str);
    exit(1);
}

struct cardz {
    char value[255];
    char color[255];
    int point_value;
} card[108];

const size_t card_sz = 108;
    
int main()
{
    void init_cardz();
    void get_cardz(FILE *f_in);
    void print_cardz();
    
    init_cardz();
    get_cardz(stdin);
    print_cardz();
}

void init_cardz()
{
    for (size_t i = 0; i < card_sz; i++) {
        strncpy(card[i].value, "", 255);
        strncpy(card[i].color, "", 255);
        card[i].point_value = 0;
    }
}

void get_cardz(FILE *f_in) 
{
    char line[255];
    for (size_t i = 0;
        strncpy(line, "", sizeof(line)),
        fgets (line, sizeof(line), f_in) != NULL;
        i++
    ) {
        // Check if too many lines
        if (i >= card_sz) {
            error(i+1, line);
        }
        // Check if line is too long
        if (line[sizeof(line)-1] != '\0') {
            error(i+1, line);
        }
        // Process line
        char dummy;
        int retval = sscanf (line, "%s %s %d %c", card[i].color, 
            card[i].value, 
            &card[i].point_value, 
            &dummy);
        // Check if wrong number of parameters
        if (retval != 3) {
            error(i+1, line);
        }
    }
}

void print_cardz()
{
    bool card_empty(struct cardz card);
    
    for (size_t i = 0; i < card_sz; i++) {
        // stop at first empty card
        if (card_empty(card[i])) {
            break;
        }
        printf("Card %d: %s, %s, %d\n", i, card[i].value, card[i].color, card[i].point_value);
    }
}

bool card_empty(struct cardz card)
{
    return strcmp(card.value, "") == 0 && 
        strcmp(card.color, "") == 0 && 
        card.point_value == 0;
}

a for loop is just a while loop with syntactic sugar. Consider this

Code:

int i=0;
while (i < 100) {
   do_something();
   i++;
}

The reason a for loop is preferred is to make all loop control together. Consider if do_something() is longer. Maybe you'll forget to increment i.

Code:

for (int i=0; i < 100; i++) {
    do_something();
}

As for init_cards in my example, it is to initialize each card to a known state. Maybe it's not necessary in this case because you allocated card on the stack, but in general you should initialize variables. For an array of structs I think it's most convenient to make a simple function to do this work. It's quite boring to type in a long list of initial values for each struct member, although this is also possible.

Let's start with the simplest fgets loop possible that will also allow you to keep track of the line numbers:

Code:

for (int lineno = 0; fgets(line, sizeof(line), f_in) != NULL; lineno++) 
{
    /* do something with line and lineno */
}

This will work but it has the problem for me that it makes checking for errors difficult. If the line is too long, then fgets will not put a '\0' as the last character in the buffer. However, if the line is too short, then the last character in the buffer is undefined. I would like to be able to do a simple test like this:

Code:

if last character in line is not '\0':
    print error message "buffer overflow"
    end program

To do this you can initialize "line" beforehand. One way is to use strncpy. If you look at the reference page for strncpy, the behaviour states that the entire buffer will be initialized with '\0' if you pass it an empty string. So:

Code:

for (int i = 0;
        strncpy(line, "", sizeof(line)),
        fgets(line, sizeof(line), f_in) != NULL;
        i++
    )
{
    /* do something with line and lineno */
}

This is the same except now it is possible to tell whether the line overflows the buffer or not, by checking line[sizeof(line)-1] and seeing if this is '\0'. If it is not '\0' then it means fgets ran out of room.

The exact meaning of , in this instance is

expr1(), expr2(), expr3(), ..., exprn()

it means each expr1, ..., exprn will be evaluated, but only the last expr's return value is used. In the case of a while loop, it means that exprn() will be the one that decides whether the loop continues or not. The previous expressions will be evaluated but are not used for loop control purposes.

anduril462 is right. Let's just simplify it to this:

Code:

for (int i = 0; fgets (line, sizeof(line), f_in) != NULL; i++) {
		// Check if too many lines
		if (i >= D) {
			exit(1);
		}
		// Check if line is too long
		if (strlen(line) == sizeof(line)-1) {
			exit(2);
		}
		// Process line
		/* ... */
	}