6
\$\begingroup\$

Exercise 7-6 from the K&R book. Compares 2 files, printing the line number and content with the first dissimilarity. What am I doing that could be improved?

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

#define MAXLINE 100

/* compare: compare files, printing first differing line */
int main(int argc, char *argv[])
{
    FILE *fp1, *fp2;
    char line1[MAXLINE], line2[MAXLINE];
    int i;
    char *prog = argv[0];

    /*require 2 files to comp*/
    if (argc != 3)
    {
        fprintf(stderr, "Incorrrect usage: got %d arguments, expected 3", argc);
        exit(1);
    }
    /*attempt to open files*/
    if (!( (fp1 = fopen(*++argv, "r")) && (fp2 = fopen(*++argv, "r")) ))
    {
        fprintf(stderr, "%s: can't open %s\n", prog, argv[1] || argv[2]);
        exit(2);
    }
    /*read line from each file*/
    for (i = 0; fgets(line1, MAXLINE, fp1) && fgets(line2, MAXLINE, fp2); i++)
    {
        if (strcmp(line1, line2))
        {
            /* print line # and line that differed */
            fprintf(stdout, "Line %d\n%s: %s\n%s: %s", i, argv[1], line1, argv[2], line2);
            break;
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

Perfect code does not exist, it's all a matter of opinion, but I would write the handling of the program arguments differently. The first argument is the name of the program so you dont expect 3 arguments, but just 2 from the user's perspective. Also, I would change the *++argv x2 into argv[1] and argv[2] to make more clear what you mean with the program arguments.

And I would explicitly check the retrun value of strcmp to make it more clear that you are checking for inequality.

So not if (strcmp()) but if (strcmp() != 0)

    if (argc != 3)
    {
        fprintf(stderr, "Incorrrect usage: got %d arguments, expected TWO", argc);
        exit(1);
    }
    /*attempt to open files*/
    if (!( (fp1 = fopen(argv[1], "r")) && (fp2 = fopen(argv[2], "r")) ))
    {
        fprintf(stderr, "%s: can't open %s\n", prog, argv[1] || argv[2]);
        exit(2);
    }
    /*read line from each file*/
    for (i = 0; fgets(line1, MAXLINE, fp1) && fgets(line2, MAXLINE, fp2); i++)
    {
        if (strcmp(line1, line2) != 0)
        {
            /* print line # and line that differed */
            fprintf(stdout, "Line %d\n%s: %s\n%s: %s", i, argv[1], line1, argv[2], line2);
            break;
        }
    }
\$\endgroup\$
3
\$\begingroup\$

This is a good, easy-to-read code. I do see a few things that I would change, though.

Error Handling

It's great that you thought to include error handling! Most beginners don't think of that.

I would look at other command-line tools to see how they display errors and then think about whether you can imitate that and whether it can be improved even more. For example, looking at a random tool like "grep," if you type grep -4 (so not a valid command line argument), you get this output:

usage: grep [-abcDEFGHhIiJLlmnOoqRSsUVvwxZ] [-A num] [-B num] [-C[num]] [-e pattern] [-f file] [--binary-files=value] [--color=when] [--context[=num]] [--directories=action] [--label] [--line-buffered] [--null] [pattern] [file ...]

OK, a little verbose, but it gives you an idea. I think for yours, it could be improved by making the error message look something like this:

usage: [app name] file1 file2

So your code for that would just be:

fprintf(stderr, "usage: %s file1 file2", prog);

Also for the file opening, I'd test opening each file separately so you can tell the user specifically which file failed to open. I would make it something like this:

if ((fp1 = fopen(*argv[1], "r")) == NULL)
{
    fprintf(stderr, "%s: can't open %s\n", prog, argv[1]);
    exit(2);
}
if ((fp2 = fopen(*argv[2], "r")) == NULL)
{
    fprintf(stderr, "%s: can't open %s\n", prog, argv[2]);
    exit(2);
}

That's a little more user-friendly, and not that much more code.

Bugs

There's a problem with your error handling in the file opening portion. Don't feel bad, though – getting error handling right is one of the hardest parts of programming.

First, as mentioned in other answers, it's not a good idea to mutate argv. It's confusing to anyone who reads it because it's not done very frequently, and you're messing with memory that someone else (the operating system) allocated, which can be dangerous if you're not very careful.

None of the above is technically an error. But the line to print the error message has one. It's not clear what your intent was in writing the fprintf() statement. But the last argument probably doesn't do what you think for a variety of reasons.

The || operator is a "logical or" operator. It returns a value that means either true or false. But your format string, %s: can't open %s\n, expects a string for the second argument. So it's going to treat that true/false value (likely a 0 or 1) as a pointer to a string. And that will likely crash.

But there's a further problem with the print statement, and with the final print statement, too. Back in the line where you open the files, you incremented argv. So now argv[1] and argv[2] point past the end of the original argv array! This will cause a crash or garbage to be printed in the final fprintf() statement, and in fact, when I run it, I do get garbage printed! You might get lucky and argv[1] and argv[2] point to a NUL character ('\0'). But that's a best-case scenario.

\$\endgroup\$
1
\$\begingroup\$

A user would expect line numbering to start with 1. Counting from 0 is a programmers' convention that is not user-friendly.

If one file terminates before the other, I would expect your program to report that fact, rather than printing nothing (which I would interpret to mean that the two files are identical).

\$\endgroup\$
1
\$\begingroup\$
#include <stdio.h>
#include <string.h>
#include <stdlib.h>

The headers from the C standard library should be included in alphabetical order. Just for consistency, since the order doesn't matter. (This only applies for the C standard library; other libraries may have other requirements.)

#define MAXLINE 100

I have lots of files with lines longer than 100 bytes. This number probably originated in the small computers from the 1980s aera. Nowadays, lines can be arbitrarily long (especially compressed XML and JSON).

/* compare: compare files, printing first differing line */
int main(int argc, char *argv[])
{
    FILE *fp1, *fp2;
    char line1[MAXLINE], line2[MAXLINE];
    int i;
    char *prog = argv[0];

    /*require 2 files to comp*/
    if (argc != 3)
    {
        fprintf(stderr, "Incorrrect usage: got %d arguments, expected 3", argc);

Instead of describing what is wrong, rather describe what the user of the program should do instead. The standard form is:

fprintf(stderr, "usage: %s <file1> <file2>\n", prog);

Note the newline at the end of the string. This is necessary.

        exit(1);
    }
    /*attempt to open files*/
    if (!( (fp1 = fopen(*++argv, "r")) && (fp2 = fopen(*++argv, "r")) ))

You should never modify the parameters of a function. Otherwise you quickly get confused. Like here, where you access argv[2] after incrementing it twice. In the unmodified form, this corresponds to argv[4], which is not defined.

    {
        fprintf(stderr, "%s: can't open %s\n", prog, argv[1] || argv[2]);
        exit(2);
    }
    /*read line from each file*/
    for (i = 0; fgets(line1, MAXLINE, fp1) && fgets(line2, MAXLINE, fp2); i++)
    {
        if (strcmp(line1, line2))
        {
            /* print line # and line that differed */
            fprintf(stdout, "Line %d\n%s: %s\n%s: %s", i, argv[1], line1, argv[2], line2);

You should use one fprintf statement per line of output. Then the code corresponds nicely to what you see on the screen. It would look like this:

printf("Line %d\n", i);
printf("%s: %s\n", argv[1], line1);
printf("%s: %s\n", argv[2], line2);

Look how it is immediately apparent from the code that the two lines are output in exactly the same format. And that the output consists of three lines. By the way, I added the missing newline at the end, like above.

            break;
        }
    }
}

And, last but most important: Always compile your code with the highest available level of compiler warnings before posting it here. It will show you obvious bugs. In this case:

gcc -Wall -Wextra -Os -c diffline.c

diffline.c: In function 'main':
diffline.c:24:25: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'int' [-Wformat=]
         fprintf(stderr, "%s: can't open %s\n", prog, argv[1] || argv[2]);
                         ^
diffline.c:24:25: warning: format '%s' expects argument of type 'char *', but argument 4 has type 'int' [-Wformat=]

If you don't understand these messages, it's OK to ask other people about them. But it is not OK to publish code with this easy to find bug. Fixing this is your task.

\$\endgroup\$
0
\$\begingroup\$

Bug

When files differ by 1 having extra lines, that case is not detected.

Note: No need to be skimpy on line type - use a wider type.

Note: No need scant buffer size, use a generous buffer.

#define MAXLINE (16*1024 + 1)

unsigned long long i = 0;
char *l1;
char *l2;
for (;;) {
  i++;  // Let first line be # 1
  l1 = fgets(line1, MAXLINE, fp1);
  l2 = fgets(line2, MAXLINE, fp2);
  if ((l1 == NULL) || (l2 == NULL) || (strcmp(l1,l2) != 0)) break;
}
// If loop ended due to an error or difference
if (!feof(fp1) || !feof(fp2)) {
  printf("Line %llu\n", i);
  if (ferror(fp1)) {
    printf("Error in %s\n", argv[1]);
  } else if(ferror(fp2)) {
    printf("Error in %s\n", argv[2]);
  } else if(feof(fp2)) {
    printf("%s longer\n", argv[1]);
  } else if(feof(fp1)) {
    printf("%s longer\n", argv[2]);
  } else {
    printf("lines differ ...\n", ...);
  }
  return EXIT_FAIL;
}
return EXIT_SUCCESS;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.