Re: [dev] print utility

From: Calvin Morrison <mutantturkey_AT_gmail.com>
Date: Sat, 30 Mar 2013 22:53:50 -0400

On 30 March 2013 22:30, Robert Ransom <rransom.8774_AT_gmail.com> wrote:
> 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.

A bit of both really, a good code review is very nice for me to learn,
and it has a purpose, not some silly lesson from a text-book without a
real use. How many times can I say hello to the world?

> 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.)

I fixed this, thank you.

> * 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.)

So would it be proper to handle it with a few if statements?

if argc is 2 try to read standard input
if argc is 3 read argv[2]
if argc is 3+ or 1, print to stderr a usage statement

if a switch appropriate? else if?

> * 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.)

I am having trouble with understanding this, do you mean something to
the effect of this?

cat print.c | print 2
#include <stdio.h>


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

I fixed this

Which is preferable? I suppose in this example it does not matter
because returning is the same as exit from the main function?

> * 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.

Should I have a seperate declaration and then intialization later or
just combine the statements:

int line = atoi(argv[2]);

or

int line;
line = atoi(argv[2]);

> * 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.)

I will look at strtoul, though I don't know anything about error
checking. Can error checking be done with atoi?

> * 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.

I have added a check.

> * 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.
>

Forget this whole LINE_MAX thing, I am looking at your suggestion in
the other message.

Thank you for the thorough review,
Calvin
Received on Sun Mar 31 2013 - 04:53:50 CEST