bildad said:
This is my solution, after research. Criticism welcome.
Not bad, but I do have a few comments.
#include <stdio.h>
#define MAX_LEN 120
Obviously this is arbitrary (as it must be). If you haven't already,
you should think about what happens if the input file contains lines
longer than MAX_LEN characters. Since you're using fgets(), the
answer is that it works anyway, but you should understand why. Read
the documentation for fgets() and work through what happens if an
input line is very long.
void ReadFile(FILE *fp);
int ErrorMsg(char *str);
int main(void)
{
FILE *fp;
char filename[]= "test.txt";
In a real program, you'd want to be able to specify the name of the
file, probably on the command line.
if ((fp = fopen(filename, "r")) == NULL){
ErrorMsg(filename);
The ErrorMsg() function claims to return an int (but see below), but
you discard the result.
} else {
ReadFile(fp);
fclose(fp);
}
return 0;
This would be a good opportunity to indicate to the environment
whether you were able to open the file, using "return EXIT_SUCCESS" if
you were successful, "return EXIT_FAILURE" if you weren't (or,
equivalently, "exit(EXIT_SUCCESS)" or "exit(EXIT_FAILURE)"). Note
that the exit() function and the EXIT_SUCCESS and EXIT_FAILURE macros
}
void ReadFile(FILE *fp)
{
char buff[MAX_LEN];
while (fgets(buff, MAX_LEN, fp)) {
Some people (including me) would prefer an explicit comparison against
NULL. What you've written is fine, though, and any C programmer
should be able to read both forms easily.
Since you're not doing any formatting of the output, "fputs(buff);"
would be simpler; it's also more closely parallel with fgets(). This
is a matter of style, though. It's often easier to use printf()
consistently than to remember the details of puts() vs. fputs() (as
well as fputc(), putc(), and putchar()).
As a matter of style, it might make more sense to pass the file name
as an argument to ReadFile(), and make FP local to it rather than to
main(). The fopen() call would then be inside ReadFile(). Also,
CopyFile() (or copy_file()) might be a better name, since it doesn't
just read the file.
int ErrorMsg(char *str)
{
printf("Cannot open %s.\n", str);
return;
}
Unless you're going to add more to this, I'm not sure it needs to be a
function. You might as well just replace the call to ErrorMsg() with
the printf() call. Also, it's traditional to write error messages to
stderr:
fprintf(stderr, "Cannot open %s\n", str);
Note that I've dropped the '.' in the error message; it could look
like it's part of the file name.
The ErrorMsg() function is declared to return an int, but you don't
return a value. In fact, this is illegal (at least in C99). You
should declare the function to return void, not int.
The return statement isn't even necessary. A return with no value is
equivalent to falling off the end of the function, which you're about
to do anyway. (Were you expecting the return to terminate the main
program? It doesn't; it just returns control to the point after the
call.)
