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? > 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
