Re: [dev] print utility

From: Robert Ransom <rransom.8774_AT_gmail.com>
Date: Sat, 30 Mar 2013 19:30:49 -0700

On 3/30/13, Calvin Morrison <mutantturkey_AT_gmail.com> wrote:
> What do you guys think of the tool? Of the code? It does one thing and
> one thing well.

Or perhaps you're just learning C and wanted someone to review your code.

Style issues:

* Error messages should be sent to stderr (s/printf(/fprintf(stderr, /
on error-message lines). (If you're not using stdio.h, that's fd 2.)

* If you're writing a quick program that no one else will have to use
as a tool, positional arguments are acceptable, but the argument that
could best be generalized to zero or multiple instances should be last
on the command line if possible. (If you had put the filename second
instead of first, it would be much easier to fix the program to
support reading from stdin.)

* If you're writing a quick program with at most one input stream and
at most one output stream, use stdin and stdout, and omit the
filename-handling code. (That's certainly faster.)

* You're using both exit() and return in main. This is noticeably inconsistent.

* Don't print trailing spaces before your newlines, unless you have an
obvious reason to. (If you hadn't had the format-string bug that
Christian noticed, you would probably have appended a trailing space
to the line that you printed from the input file instead.)

* You're initializing line in its declaration, but then
re-initializing it before it is read from. That's silly -- omit the
first initialization.

* Your usage message should be a string literal in the fprintf or
fputs call that prints it, which should in turn be in a separate
usage() function. (Usually you'll need to print the usage message in
more than one case.)

Correctness issues:

* You're setting a variable of type char * (usage) to the const char *
address of a string literal.

* Your program continues to read the whole file even after it finds
and prints the line that the user asked for. This would be especially
annoying if the input were piped from a program that spews forever.

* 'print foo bar baz' will (a) not complain that it was given more
than two arguments, and (b) choose an unspecified line, depending on
how your libc handles errors in atoi. (strtoul is better. Remember
to detect errors by checking errno.)

* Your program will silently print nothing if the line number is (a
string which atoi parses to) a non-positive number. That's probably
an error that you should print a message about.

* LINE_MAX is a bug, not a feature. If you really can't be bothered
to support arbitrarily long lines, at least define your maximum line
length explicitly.

* If you think your program is a 'utility' which processes 'text
files' as specified by POSIX, your fgets buffer needs to be at least
LINE_MAX+1 bytes long to accommodate a trailing NUL.

* If LINE_MAX is large enough to be reasonable, you probably shouldn't
allocate LINE_MAX bytes on the stack. Use a heap-allocated buffer or
a static buffer.


Robert Ransom
Received on Sun Mar 31 2013 - 04:30:49 CEST