On 3/30/13, Calvin Morrison <mutanttur...@gmail.com> wrote: > On 30 March 2013 22:30, Robert Ransom <rransom.8...@gmail.com> wrote: >> On 3/30/13, Calvin Morrison <mutanttur...@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