Why my string getting some extra value? (C program)

Question

I am trying to solve a C Program problem:

Create a program in C that reads a string from a text file and then reorders the string in an odd-even format (first take odd numbered letters and then even numbered letters; example: if the program reads elephant, then the reordered string will be eehnlpat). Then write the string in a different text file. Provide an error-checking mechanism for both reading and writing.

My code is:

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

int main() {
    FILE *inputFile;

    inputFile = fopen("inpFile.txt", "r");

    if (inputFile != NULL) {
        FILE *outFile = fopen("outFile.txt", "w");

        if (outFile != NULL) {
            printf("file created successfully\n");

            int i, j = 0;
            char strf1[50];
            fscanf(inputFile, "%s", &strf1);
            char strf2[strlen(strf1)];
            for (i = 0; strf1[i] > 0; i++) {
                if (i % 2 == 0) {
                    strf2[j] = strf1[i];
                    j++;
                }
            }
            for (i = 1; strf1[i] > 0; i++) {
                if (i % 2 == 1) {
                    strf2[j] = strf1[i];
                    j++;
                }
            }
            fprintf(outFile, "%s\n", strf2);

            fclose(outFile);
        } else {
            printf("file could not be created\n");
        }
        fclose(inputFile);
    } else {
        printf("File does not exist.");
    }
    return 0;
}

I feel all is OK but the problem is if the program reads elephant, then the reordered string given by my program is eehnlpatZ0@. Where extra Z0@ is my problem. I don't want that extra thing. But I can't fix it. If anybody can help me to fix it, that will be great.

Answer 1

Your target string is too short: char strf2[strlen(strf1)];. You should at least allow for a null terminator and set it, or simply make the output array the same size as the input array:

char strf2[50];

There are other problems in your code:

• In case of error by fopen, it would be advisable to return a non-zero status to the system.

• You should pass the array to fscanf(), not a pointer to the array, which has a different type.

• You should tell fscanf() the maximum number of characters to read into the array with %49s

• You should test the return value of fscanf() and produce an empty output file for an empty input file. The current code has undefined behavior in this case.

• The test strf1[i] > 0 is incorrect: characters from the input file might be negative. You should either compute the string length or test with strf1[i] != '\0'

• Starting the second loop at i = 1 seems a good idea, but it relies on the silent assumption that strf1 is not an empty string. In your example, if fscanf() succeeds, strf1 is not empty, and if it fails the behavior is undefined because strf1 is uninitialized. Yet it is safer to avoid such optimisations which will bite you if you later move the code to a generic function for which the assumption might not hold.

• You must null terminate the output string before passing it to fprintf or specify the length with a %.*s format.

Here is a corrected version:

#include <stdio.h>

int main() {
    FILE *inputFile, *outFile;
    char strf1[50], strf2[50];
    int i, j;

    inputFile = fopen("inpFile.txt", "r");
    if (inputFile == NULL) {
        printf("Cannot open input file inpFile.txt\n");
        return 1;
    }
    outFile = fopen("outFile.txt", "w");
    if (outFile == NULL) {
        printf("Could not create output file outFile.txt\n");
        fclose(inputFile);
        return 1;
    }
    printf("file created successfully\n");

    if (fscanf(inputFile, "%49s", strf1) == 1) {
        j = 0;
        for (i = 0; strf1[i] != '\0'; i++) {
            if (i % 2 == 0)
                strf2[j++] = strf1[i];
        }
        for (i = 0; strf1[i] != '\0'; i++) {
            if (i % 2 == 1) 
                strf2[j++] = strf1[i];
        }
        strf2[j] = '\0';
        fprintf(outFile, "%s\n", strf2);
    }
    fclose(inputFile);
    fclose(outFile);
    return 0;
}

Here is an alternative with simpler copy loops:

        int len = strlen(strf1);
        j = 0;
        for (i = 0; i < len; i += 2) {
            strf2[j++] = strf1[i];
        }
        for (i = 1; i < len; i += 2) {
            strf2[j++] = strf1[i];
        }
        strf2[j] = '\0';

Answer 2

You have to provide a space for the null-terminator, since you did not provide a space for it, printf cannot know when your string is terminated, so it contiues to print out data from the memory.

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


int main()
{
    FILE* inputFile;

    inputFile=fopen("inpFile.txt", "r");

    if (inputFile!=NULL) {
        FILE* outFile=fopen("outFile.txt", "w");

        if (outFile!=NULL) {
            printf("file created successfully\n");

            int i, j=0;
            char strf1[50];
            fscanf(inputFile, "%s",&strf1);
            int inputLength = strlen(strf1) + 1;
            char strf2[inputLength];
            char strf2[inputLength-1] = '\0';

            for(i=0; strf1[i]>0; i++) {
                if(i%2==0) {
                    strf2[j]=strf1[i];
                    j++;
                }
            }
            for(i=1; strf1[i]>0; i++) {
                if(i%2==1) {
                    strf2[j]=strf1[i];
                    j++;
                }
            }
            fprintf(outFile, "%s\n",strf2);

            fclose(outFile);
        }else{
            printf("file could not be created\n");
        }

        fclose(inputFile);
    }
    else {
        printf("File does not exist.");
    }
    return 0;
}

Answer 3

In C, strings require a Null character, '\0', as the last byte in order to terminate.

Changing the following line of code from

char strf2[strlen(strf1)];

to

char strf2[strlen(strf1) + 1];

will solve this problem.