While loop in C Language with feof condition and fgets elaborate twice for no reason?

Question

I have a text file with a lots of records (about 20k lines) that is written like this:

00000000090020120200728012
00000010520020120200729012
00000002740020120200729012
00000002710020120200736012
00000001601020120200755012
00000002870020120200758012
00000010690020120200753013
00000001760020120200800013
00000008480020120200800013
00000009370020120200733014
00000001500020120200739014
00000012400020120200743014
00000008720020120200715015
00009100570020120200734017
00000002060020120200734017
00000002050020120200734017
00000003670020120200734017

these records contain data information of accesses of office in year 2020, and every record can be readed with this structure (im gonna take the first line as example):

reading a string character by character, the record is splitted in this way:

0000 (index [0-3] -> useless data)
000009 (index [4-9] -> Badge ID)
0 (index [10] -> acts like boolean, 0 for in - access - and 1 for out - exit -)
02012020 (index [11-18] -> date format)
0728 (index [19-23] -> hours and minutes of the access)
012 (index [24-26] -> just an information about the gate ID)

i have this code that i wrote for counting the records for a specified badge ID on a specified gate (in my case i need to read only 001, 002, 003 gates):

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

#define NUMLEN 27

int main () {
    printf("\n-- 32bit version\n");
    int entriesCounter = 0;
    char buff[NUMLEN];
    char requiredBadge[7];
    char badge[7];
    char entry[2];
    char day[3];
    char month[3];
    char year[5];
    char hours[3];
    char minutes[3];
    char gate[4];
    FILE *fp;
    fp = fopen("Storico2020.txt", "r");
    if (fp == NULL) {
        printf("Error open");
        exit(1);
    }
    printf("\nInsert ID badge for counting accesses: ");
    scanf("%s", requiredBadge);
    while(!feof(fp)) {
        fgets(buff, NUMLEN, fp);    // example -> init:0000 | badge:000352 | entry:1 | data:01012019 | time:0030 | gate:023
        strncpy(badge, buff+4, 6);
        badge[6] = '\0';
        strncpy(entry, buff+10, 1);
        entry[1] = '\0';
        strncpy(day, buff+11, 2);
        day[2] = '\0';
        strncpy(month, buff+13, 2);
        month[2] = '\0';
        strncpy(year, buff+15, 4);
        year[4] = '\0';
        strncpy(hours, buff+19, 2);
        hours[2] = '\0';
        strncpy(minutes, buff+21, 2);
        minutes[2] = '\0';
        strncpy(gate, buff+23, 3);
        gate[3] = '\0';
        if (strcmp(requiredBadge, badge) == 0 && strcmp(entry, "1") == 0) {
            printf("\nBadge: %s | in date: %s/%s/%s | gate: %s | hour: %s:%s", badge, day, month, year, gate, hours, minutes);
            entriesCounter++;
        }
        
    }
    fclose(fp);
    printf("\n********** TOTAL ACCESSES OF BADGE ID %s: %d ***************\n" ,requiredBadge, entriesCounter);
    
    system("PAUSE");

    return 0;
}

but, the problem here is that it counts every records that finds TWICE, and i really don't know why. i put in the if condition also the entry == 1 because i need to count once the entrance in the office, not also the exit. can you help me? for example, if u count ID badge 002341 it counts 342 accesses, but it need to count only the half. help me please!

https://stackoverflow.com/questions/5431941/why-is-while-feoffile-always-wrong — William Pursell, Oct 18 '22 at 11:31
While C++ [this answer](https://stackoverflow.com/a/5605159/1312382) still covers the same issue and explains quite nicely the issue... — Aconcagua, Oct 18 '22 at 11:38
thank you, in fact i resolved the problem increasing the NUMLEN constant number, it works now. Thank you guys and sorry again for the question, i didnt notice that mistake. — sirducas, Oct 18 '22 at 12:02
@user3121023 Indeed. Not sure how to change the duplicate, so I'll just vote to repoen. — William Pursell, Oct 18 '22 at 12:34
Since you aren't copying null-terminated strings, you should use memcpy instead of strncpy. — stark, Oct 18 '22 at 12:48

score 1 · Answer 1 · answered Oct 18 '22 at 13:22

The issue is that the buffer is too small for fgets to read all the digits and the newline in one call. fgets will read at most NUMLEN - 1 characters. #define NUMLEN 27 lets fgets read the digits in one call but there isn't room to read the newline. The newline is read in a second fgets call. Using #define NUMLEN 30 would provide a large enough buffer to read the digits and the newline in one fgets call.
Using !feof(fp) as the while condition can be a problem. When the last line is read there is no error so the loop iterates again. Instead use fgets as the while condition.
Consider using sscanf to extract the fields. %2s will scan up to two characters.

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

#define NUMLEN 30

int main () {
    printf("\n-- 32bit version\n");
    int entriesCounter = 0;
    char const *filename = "Storico2020.txt";
    char buff[NUMLEN] = "";
    char requiredBadge[7] = "";
    char badge[7] = "";
    char entry[2] = "";
    char day[3] = "";
    char month[3] = "";
    char year[5] = "";
    char hours[3] = "";
    char minutes[3] = "";
    char gate[4] = "";
    FILE *fp = NULL;

    fp = fopen( filename, "r");
    if (fp == NULL) {
        perror ( filename);
        exit(1);
    }
    printf("\nInsert ID badge for counting accesses: ");
    scanf("%6s", requiredBadge);
    while( fgets(buff, NUMLEN, fp)) {
        // example -> init:0000 | badge:000352 | entry:1 | data:01012019 | time:0030 | gate:023
        if ( 8 != sscanf ( buff + 4, "%6s%1s%2s%2s%4s%2s%2s%3s"
        , badge
        , entry
        , day
        , month
        , year
        , hours
        , minutes
        , gate)) {
            fprintf ( stderr, "bad record %s\n", buff);
            continue;
        }
        if (strcmp(requiredBadge, badge) == 0 && strcmp(entry, "1") == 0) {
            printf("\nBadge: %s | in date: %s/%s/%s | gate: %s | hour: %s:%s", badge, day, month, year, gate, hours, minutes);
            entriesCounter++;
        }

    }
    fclose(fp);
    printf("\n********** TOTAL ACCESSES OF BADGE ID %s: %d ***************\n" ,requiredBadge, entriesCounter);

    // system("PAUSE");

    return 0;
}

William Pursell · Answer 2 · 2022-10-18T19:16:54.470

You need to be more careful about validating input. Always (always) check the value returned by scanf. And, although the code below is not how I would do this, it pains me to see you wantonly copying all that data around. You don't need to. If you are copying data just because you want to have null terminated strings but don't want to destroy the buffer, you should rethink things. eg:

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

void
show(const char *header, const char *str, size_t len)
{
    fputs(header, stdout);
    fwrite(str, 1, len, stdout);
}

int
main(int argc, char **argv)
{
    char buff[1024];
    unsigned line = 0;
    unsigned entriesCounter = 0;
    const char * const badge = buff + 4;
    const char * const entry = buff + 10;
    const char * const day = buff + 11;
    const char * const month = buff + 13;
    const char * const year = buff + 15;
    const char * const hours = buff + 19;
    const char * const minutes = buff + 21;
    const char * const gate = buff + 23;
    char * const end = buff + 26;

    if( argc < 2 ){
        fprintf(stderr, "missing badge argument\n");
        exit(EXIT_FAILURE);
    }
    const char *requiredBadge = argv[1];
    const  char *path = argc > 2 ? argv[2] : "stdin";
    FILE  *fp = argc > 2 ? fopen(argv[2], "r") : stdin;

    if( fp == NULL ){
        perror(path);
        exit(1);
    }

    while( *end = '\0', fgets(buff, sizeof buff, fp) != NULL ){
        line += 1;
        if( *end != '\n' ){
            fprintf(stderr, "Unexpected input in line %u\n", line);
            continue;
        }
        if( strncmp(requiredBadge, badge, entry - badge) == 0
            && *entry == '1'
        ) {
            printf("%5d: ", ++entriesCounter);
            show("Badge: ", badge, entry - badge);
            show(" | in date: ", day, month - day);
            show("/", month, year - month);
            show("/", year, hours - year);
            show(" | gate: ", gate, end - gate);
            show(" | hour: ", hours, minutes - hours);
            show(":", minutes, gate - minutes);
            putchar('\n');
        }
    }
}

/* Sample input:
00000010520020120200729012
00000002740020120200729012
00000002711020120200736012
00000002710020120200736012
00000002711020120200736012
00000001601020120200755012
00000002870020120200758012
00000010690020120200753013
00000001760020120200800013
00000008480020120200800013
00000009370020120200733014
*/

Note that this tweaks the interface a bit, and reads the desired badge as a command line argument rather than taking it from the input stream.

While loop in C Language with feof condition and fgets elaborate twice for no reason?

2 Answers2