Merging two files' lines of text for output

Question

My program is supposed to merge two text files' lines together. For instance, if I have two files, one.txt:

a b c d e f g h i j k l m
n o p q r s t u v w x y z

and two.txt:

0 1 2 3 4
5 6 7 8 9

The output is:

a b c d e f g h i j k l m0 1 2 3 4
n o p q r s t u v w x y z5 6 7 8 9 

However, I feel as though some of the conditions are redundant. However, I'm not sure how I can better design this.

// Program to merge lines from two files and output results
#include <stdio.h>

int main(int argc, char *argv[])
{
    char *inName1 = argv[1], *inName2 = argv[2];
    FILE *in1, *in2;
    int c, d;

    // Ensure correct usage
    if (argc != 3)
    {
        fprintf(stderr, "Usage: ./merge <file1> <file2>\n");
        return 1;
    }

    // Open input files and return if unable to open
    if ((in1 = fopen(inName1, "r")) == NULL)
    {
        fprintf(stderr, "Can't open %s.\n", inName1);
        return 2;
    }

    if ((in2 = fopen(inName2, "r")) == NULL)
    {
        fprintf(stderr, "Can't open %s.\n", inName2);
        return 3;
    }

    // Take care of output
    while ((c = getc(in1)) != EOF)
    {
        if (c != '\n')
            putc(c, stdout);        
        else
        {
            while ((d = getc(in2)) != EOF)
            {
                if (d != '\n')
                    putc(d, stdout);
                else
                {
                    putc('\n', stdout);
                    break;
                }
            }

            if (d == EOF)
            {
                while (c != EOF)
                {
                    putc(c, stdout);
                    c = getc(in1);
                }
            }
        }
    }

    if (c == EOF)
    {
        while ((d = getc(in2)) != EOF)
        {
            putc(d, stdout);
        }
    }

    fclose(in1);
    fclose(in2);

    printf("\nProcess ended successfully.\n");

    return 0;
}

Answer 1

Some good things

This compiles cleanly with all my usual compiler warnings enabled - well done!

You've used stderr appropriately for the output (with the exception of the final message); that's good practice.

Don't dereference argv before you've checked its bounds

This is wrong:

    char *inName1 = argv[1], *inName2 = argv[2];

    if (argc != 3) {
        fprintf(stderr, "Usage: ./merge <file1> <file2>\n");
        return 1;
    }

We can't read from argv[1] or argv[2] until after we know that argc is big enough:

    if (argc != 3)
    {
        fprintf(stderr, "Usage: %s <file1> <file2>\n", argv[0]);
        return 1;
    }
    char *inName1 = argv[1], *inName2 = argv[2];

I've enhanced your error message to use the actual name of the program, as passed in argv[0].

Report actual error type on failure

"Can't open " at least specifies which file couldn't be opened, but we can be more specific (file not found, permission denied, ...) using the perror() function:

    FILE *const in1 = fopen(inName1, "r");
    if (!in1) {
        perror(inName1);
        return 1;
    }

I also changed this to return 1 from any error - there isn't any great benefit in distinguishing the different causes in the error code.

A slight tweak to the logic

The loops look mostly correct; there's a small duplication we can avoid here:

        while ((d = getc(in2)) != EOF)
        {
            if (d != '\n')
                putc(d, stdout);
            else
            {
                putc('\n', stdout);
                break;
            }
        }

We can bring the putc() outside the if-else like this:

        while ((d = getc(in2)) != EOF) {
            putc(d, stdout);
            if (d == '\n') {
                break;
            }
        }

A redundant test

while ((c = getc(in1)) != EOF)
{
    /* ... */
}

/* c can only be EOF here */
if (c == EOF)
{
    /* ... */
}

Consider some edge cases

You've taken good care to ensure that when one file ends, the rest of the other is output. Have you thought about when to output a newline, if the ending file doesn't itself end with one. It's not clear what you intend to happen in that case - comments should explain the desired output.

Alternative logic

I prefer a less deeply nested set of loops, where the first loop is the "normal" case where we're reading from both files, and we leave that loop when either file is exhausted, and finish copying from the other:

int c, d;
// Take care of output
while ((c = getc(in1)) != EOF) {
    if (c != '\n') {
        putc(c, stdout);
        continue;
    }

    while ((d = getc(in2)) != EOF) {
        putc(d, stdout);
        if (d == '\n') {
            break;
        }
    }

    if (d == EOF) {
        break;
    }
}

while ((c = getc(in1)) != EOF) {
    putc(c, stdout);
}
while ((d = getc(in2)) != EOF) {
    putc(d, stdout);
}

I've kept both c and d variables for this, although we only really need one for both files.

Error checking of output

None of the above code ever checks the return value of putc(). Filesystems do fill up, and users do disconnect streams and worse, so we really ought to ensure that the writes succeed.

Error checking of input

We currently assume that EOF returned from getc() means that we've reached the end of the file. However, it could mean that we have an error of reading, and we should distinguish those cases using feof() or ferror().

Final message

printf("\nProcess ended successfully.\n");

This isn't part of the output (and you don't want it to be passed along in a pipeline). Send status messages to stderr - or better still, be silent whilst successful, like the standard utilities.

Answer 2

Use functions to avoid repeating code

You have one big main() function that does everything. While this is a small program, it still is better to split off some logical pieces of code into their own functions. In this case, an operation that you repeat several times is copying one line of input from one input file to stdout. So write a function like this:

int copyline(FILE *in, FILE *out) {
     int c;

     // Copy all characters up to a '\n' or EOF
     while ((c = getc(in)) != '\n' && c != EOF) {
         putc(c, out);
     }

     // This will return either '\n' or EOF
     return c;
}

Note how this function returns the last result from getc(). In main, you can use it to simplify your code like this:

// Copy all lines from in1
while (copyline(in1, stdout) != EOF) {
    copyline(in2, stdout);
    putc('\n', stdout);
}

// Copy any remaining lines from in2
while (copyline(in2, stdout) != EOF) {
    putc('\n', stdout);
}

Try to read whole lines instead of characters at a time

Now that you already have working code, you can think of optimizing it. Your code reads and writes one character at a time. This might not be ideal. Function calls like getc() and putc() are not free, and since they cannot be inlined, the compiler will have a hard time optimizing your loops.

If you can read and write whole lines at once, that would be better. If you split of your line copying code into a function like copyline() above, then you only need to upgrade that function. One possibility is to use the fgets() and fputs() functions that read and write whole lines at once. Be aware though that fgets() can only read as many characters as the size of the buffer you supply (minus one), so you need to detect and handle longer lines somehow. Also, these functions assume you are working with C strings, which use NUL bytes as terminators, and as such they will give you problems if you want your program to be able to handle lines that contain NUL bytes.

Another option is to fread() the whole file into memory, or to use mmap() to map it into memory, and implement your own function to scan the memory for newline characters.

When optimizing code, make sure that you run benchmarks on the old and new implementations, never assume that a particular implementation is faster or slower than another one without having measured it.

Close stdout and check for errors

You should call fclose(stdout) as well at the end, and check for errors. File output is buffered, and the fclose() call will ensure any buffers are flushed. Errors can definitely happen; for example, the user might have redirected stdout to a file, and the filesystem did not have enough space to hold the output.

If you silently ignore these errors, data corruption might happen more easily. For example, if you want to automate a process where two intermediate output files are generated, but you are only interested in the merged output, and will delete the temporary files afterwards, you might have a shell script that contains this line:

mergelines temp1.txt temp2.txt >result.txt && rm temp*.txt

By returning an error, you will have prevented the loss of the original files. Note, GNU Coreutils, the package that contains all the basic UNIX text processing utilities like cat, does exactly that.

Try to make your program as generic as possible

Your program can merge the lines from two files, but what if someone wants to merge the lines from three files? This sounds like a very logical extension, and if you made your program a little bit more generic, you can easily handle an arbitrary number of input files. Just make an array of FILE * pointers, as big as argc - 1, and for each filename on the command line, open that file. Then copy lines from all open files, until all of them return EOF.

You might also want to allow specifying only one filename on the command line, or even none at all!