[clean-list] Advice on style, please

[Fergus Henderson]

On 16-Dec-2001, alanh@dcs.kcl.ac.uk <alanh@dcs.kcl.ac.uk> wrote:
> All should be robustly written, with adequate error messages.
...
> #include <stdio.h>
> #include <stdlib.h>
> 
> main(int argc, char *argv[]) {

That's no longer standard-conforming.
You need to explicitly declare the return type as `int'.

> char c;
> FILE *iFile, *oFile;
> 
>   if (argc != 3) {
>     fprintf(stderr, "Usage:\ncopy <fromFile> <toFile>\n");
>     exit(1);
>   }

The use of `exit(1)' is not completely portable --
`exit(EXIT_FAILURE)' should be used instead.

>   iFile = fopen(argv[1],"r");
>   if (iFile <= 0) {

This test using `<=' on a pointer is highly non-portable.

(The code should of course be using `!=' rather than `<='...
and preferably using `NULL' rather than 0.)

>     printf("Cannot open input file %s\n", argv[1]);
>     exit(1);
>   }
>   oFile = fopen(argv[2],"w");
>   if (oFile <= 0) {
>     printf("Cannot open output file %s\n", argv[2]);
>     exit(1);
>   }
>   
>   c = getc(iFile);
>   while (c != EOF) {
>     putc(c,oFile);
>     c = getc(iFile);
>   }
> }

As well as failing on standard conformance and portability, as explained
earlier, that code fails on both the robustness and adequate error
messages criteria.

It fails on robustness because it doesn't correctly handle the case of
copying files containing characters with a byte value of 255 (on most
implementations).  In addition, on some implementations (e.g. on Windows),
the "copy" may not use the same line termination conventions as the
original file, because the file is opened in text mode rather than
binary mode.

It fails on the adequate error criteria because it only says "Cannot open
output file foo\n", but does not give any indication of *why* it failed.
The actual error could be "out of disk space", "permission denied",
"read-only file system", "network file system time-out", "I/O error",
or any of dozens of other problems, and the action that the user needs
to take will be different in each case.  Discarding this information
is a terrible lapse.

It fails again on robustness and adequate error messages because the
error messages are printed to stdout, not to stderr.

It fails yet again on robustness because the return value of putc()
is not checked, so errors writing to the output file are ignored!
This one is truly unforgivable.


I think this example demonstrates very well how C makes it easy to write
code that appears to work but which is in fact full of bugs!
Indeed, you did such a good job of it that I suspect it may have
been deliberate ;-)

-- 
Fergus Henderson <fjh@cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.