How to fix the fprintf loop?

Question

I've got a .txt file written like this:

PersonName colour1 colour2 colour3

for example:

Nick blue green black
Lisa orange yellow green
Mark white blue orange

my program: editColour(fileName, colourInTheFile, colourIWant) should change the colourInTheFile with the colourIWant and return the number of times it did it. The problem is that it starts a loop

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

typedef char String[100];

int editColour(String fileName,String current_colour,String new_colour){

    String name,c1,c2,c3;

    int num_s = 0; /*number of occurence*/

    FILE *fp = fopen(fileName,"r");
    /*this is just a test, so i'm tryng to put the fileName content in this one*/
    FILE *ftemp = fopen("/Users/valerio/Desktop/PROGRAMMI/C/Lezione12_esercizio1/Lezione12_esercizio1/file.txt","w+");
    long pos;

    while(!feof(fp)){


        pos = ftell(fp);
        fscanf(fp, "%s %s %s %s\n",name,c1,c2,c3);


        if(strcmp(c1, current_colour)==0){

            fseek(fp,pos,SEEK_SET);
            fprintf(ftemp, "%s %s %s %s\n",name,new_colour,c2,c3);
            num_s++;

        }
        if(strcmp(c2, current_colour)==0){

            fseek(fp,pos,SEEK_SET);
            fprintf(ftemp, "%s %s %s %s\n",name,c1,new_colour,c3);
            num_s++;

        }
        if(strcmp(c3, current_colour)==0){

            fseek(fp,pos,SEEK_SET);
            fprintf(ftemp, "%s %s %s %s\n",name,c1,c2,new_colour);
            num_s++;

        }

        fprintf(ftemp, "%s %s %s %s\n",name,c1,c2,c3);

    }

    fclose(fp);
    fclose(ftemp);
    return num_s;

}

int main(int argc, const char * argv[]) {

    printf("%d\n\n", editColour("/Users/valerio/Desktop/PROGRAMMI/C/Lezione12_esercizio1/Lezione12_esercizio1/social_users copia.txt",
                                    "orange", "poop"));

}

expected :

       Nick blue green black
       Lisa poop yellow green
       Mark white blue poop (in the new file)

actual :

     Nick blue green black
     Lisa poop yellow green
     Lisa orange yellow green
     Lisa poop yellow green
     Lisa orange yellow green
     Lisa poop yellow green
     Lisa orange yellow green
     Lisa poop yellow green
     Lisa orange yellow green
      .
      .  
      .

in loop

I fixed the formatting for you. You have to select your code and click the "{}" button (or, equivalently, begin each code line with four spaces). — Steve Summit, Jun 14 '19 at 16:05
[why while(!feof(fp)){ is always wrong](https://stackoverflow.com/questions/5431941/why-is-while-feoffile-always-wrong) — user3629249, Jun 14 '19 at 16:10
OT: regarding: `fscanf(fp, "%s %s %s %s\n",name,c1,c2,c3);` 1) always check the returned value (not the parameter values) to assure the operation was successful. Note in this case any returned value other than 4 indicates an error occurred 2) when using the input format specifiers '%s' and/or '%[...]' always include a MAX CHARACTERS modifier that is 1 less than the length of the input buffer because those specifiers always append a NUL byte to the input. This also avoids any buffer overflow and the resulting undefined behavior — user3629249, Jun 14 '19 at 16:15
Style suggestion: C doesn't really have a string type, so saying things like `typedef char String[100]` is (in my opinion) more confusing than clarifying. You have to manipulate strings in C "by hand", sort of, and even though they're fundamentally arrays, more often than not you manipulate them using pointers. So my advice is to explicitly use `char []` or `char *` in your code as appropriate, and drop the "convenience" typedef. — Steve Summit, Jun 14 '19 at 16:18
OT: regarding: `int main(int argc, const char * argv[]) {` When the parameters to `main()` are not going to be use, then use the signature: `int main( void )` to avoid the compiler outputting to warning messages about unused parameters — user3629249, Jun 14 '19 at 16:18
regarding; `fseek(fp,pos,SEEK_SET); fprintf(ftemp, "%s %s %s %s\n",name,c1,c2,new_colour);` This is not reliable, it will, most likely. corrupt the file. This is because not all color names are the same length. Suggest creating a 'temp' file, writing all the unchanged lines and the modified lines into it, then deleting the original file, and renaming the 'temp' file to the original file name\ — user3629249, Jun 14 '19 at 16:22
the posted code is outputting any changed line AND always outputting the original line. This will result in a larger file and other problems — user3629249, Jun 14 '19 at 16:25
regarding: `fscanf(fp, "%s %s %s %s\n",name,c1,c2,c3);` and `while(!feof(fp)){` Strongly suggest moving the call to `fscanf()` to replace the call to `feof()` in the `while()` statement — user3629249, Jun 14 '19 at 16:29
I assumed `fp` and `ftemp` were different files, since one was opened from `fileName` and the other used a (presumably different) hard-coded path. But if they're actually the same file (which is a bad idea if you can avoid it), then of course there are lots of problems. — Tom Karzes, Jun 14 '19 at 16:30

score 0 · Accepted Answer · answered Jun 14 '19 at 16:51

the following proposed code:

cleanly compiles
performs the desired functionality
properly checks for errors
avoids the incorrect statement: while( !feof(fp) )

And now, the proposed code:

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

#define MAX_CHAR 100

int editColour( char fileName[], char current_colour[], char new_colour[] )
{
    char name[ MAX_CHAR ];
    char c1[ MAX_CHAR ];
    char c2[ MAX_CHAR ];
    char c3[ MAX_CHAR ];

    int num_s = 0; /*number of occurence*/

    FILE *fp = fopen(fileName,"r");
    if( !fp )
    {
        perror( "fopen for input file failed" );
        exit( EXIT_FAILURE );
    }

    FILE *ftemp = fopen("/Users/valerio/Desktop/PROGRAMMI/C/Lezione12_esercizio1/Lezione12_esercizio1/file.txt","w");
    if( !ftemp )
    {
        perror( "fopen for output file failed" );
        fclose( fp );
        exit( EXIT_FAILURE );
    }

    while( fscanf( fp, "%99s %99s %99s %99s\n", name, c1, c2, c3 ) == 4 )
    {
        if( !strcmp( c1, current_colour ) )
        {
            fprintf( ftemp, "%s %s %s %s\n", name, new_colour, c2, c3 );
            num_s++;
        }

        if( !strcmp( c2, current_colour ) )
        {
            fprintf( ftemp, "%s %s %s %s\n", name, c1, new_colour, c3 );
            num_s++;
        }

        if( !strcmp( c3, current_colour ) )
        {
            fprintf( ftemp, "%s %s %s %s\n", name, c1, c2, new_colour );
            num_s++;
        }

        else
        {
            fprintf( ftemp, "%s %s %s %s\n", name, c1, c2, c3 );
        }

    }

    fclose(fp);
    fclose(ftemp);
    return num_s;
}


int main( void ) 
{
    printf("%d\n\n", 
        editColour("/Users/valerio/Desktop/PROGRAMMI/C/Lezione12_esercizio1/Lezione12_esercizio1/social_users copia.txt",
           "orange", "poop"));
}

thank u very much! it does exactly what i wanted, but i added a "continue" in every if statement to avoid the re-writing of the same line with the old colour name. Just a question... why did u use '%99s'? for the fscanf? — Osmion, Jun 15 '19 at 13:31
Used '%99s' because the input field is 100 bytes and need to allow room for the NUL byte that will be added to the input by the '%s' input format specifier — user3629249, Jun 15 '19 at 18:13

How to fix the fprintf loop?

1 Answers1