On 3/30/13, Calvin Morrison <mutantturkey_AT_gmail.com> wrote:
> 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?
m*n times, where m is the number of programming languages you've
started learning and n is the number of natural languages you've seen
a greeting in.
But I think I've written dumpargs.c (a test program that prints out
argc and argv[0..n]) more times than that.
>> Style issues:
>> * 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
I prefer:
FILE *input = stdin;
if (argc < 2 || argc > 3) usage();
if (argc >= 2) {
requested_line = parse_ulonglong(argv[1]);
/* detect and handle whatever errors parse_ulonglong doesn't */
};
if (argc >= 3) {
input = fopen(argv[2]);
/* detect and handle errors */
};
> if a switch appropriate? else if?
switch is trickier to use correctly. else doesn't seem useful for
argument handling.
>> * 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>
I prefer:
print 2 <print.c
>> * 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?
I prefer to use return in main(), and exit() only in functions where I
can't return an error indication (or don't want to).
>> * 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]);
This is bad in C89, because the declaration must come before you check
whether argc is at least 2. If you're using a language which allows
variable declarations anywhere within a block (e.g. C99, Go, or C++),
this is good if you can use it, but remember that a variable
declaration within an if statement will not be 'in scope' for the rest
of the function.
> or
>
> int line;
> line = atoi(argv[2]);
This would be ugly if the lines were adjacent.
>> * 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:
>> * '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.
Set errno to 0 before calling strtoul; if an error occurs, it will be
non-zero afterward. If your input string should only contain one
integer, also make sure **endptr == '\0' (but only after checking that
errno != 0).
> Can error checking be done with atoi?
Not reliably.
>> * 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.
I forgot to mention the reason for this -- I've read that at least one
OS (probably the HURD) defines LINE_MAX as something unreasonably low
(100?) (but the system utilities are GNU, and can handle lines as
large as malloc will allow), and I didn't know what LINE_MAX actually
is on any of the OSes I use. (On my Linuxes, it turns out to be
2048.)
Robert Ransom
Received on Sun Mar 31 2013 - 05:46:05 CEST