I skimmed thru the code and wrote down my initial reactions, and then followed 'em up with the mitigation or rebuttal of the criticism. Not much rebutting, and darn little mitigation going on here.
Most of it's pretty general, as I didn't have the patience to do a line-referencing full on code review. I'd've ended up rewriting it. ----------------------------------------------------------------------------- Comment #1 No overview-of-solution comment. Should at least point out that it does a depth-first search by rearraning the strings. Mitigation/Rebuttal It's a very small program. Look at the code. Comment #2 No makefile. All released C programs should presumably ship with a makefile. Mitigation/Rebuttal It compiles with gcc and no options. It doesn't need a makefile. Comment #3 All in one file. Should break it into at least four files (puzzle and queue code, plus the associated .h files.) Mitigation/Rebuttal Yes. However, this fits all in one file, which means it can be distributed without having to tar it up first. Comment #4 BLANK is a define, not a variable; if I, the user, wanted to use '_' instead of ' ' (so I wouldn't have to quote the arguments on the command line, say), I have to recompile the code. Mitigation/Rebuttal It's not a lot of code to recompile. Comment #5 Mixture of typedef, struct *, and void * types. Mitigation/Rebuttal That fell out of not having a C reference book on hand to remind me about the details of using typedef in recursive data structures; I changed my approach partway through, and again towards the end, and then abandoned further cleanup. Comment #6 Comments aren't very good. Improve comment quality. Mitigation/Rebuttal It's a scratch program. Didn't actually intend to release it. Comment #7 Cryptic / poorly named variable and type names. Mitigation/Rebuttal It's a scratch program. Didn't actually intend to release it. Comment #8 The deque_remove function does not check to see if the queue is empty. Mitigation/Rebuttal The code that uses this data structure always checks first. Comment #9 Most functions do not sanity-check their arguments. Mitigation/Rebuttal It's a scratch program. Didn't actually intend to release it. Comment #10 The function deque_size is really slow. Mitigation/Rebuttal There's a TODO in the struct that would improve performance, and this function isn't called to do anything vital, and so it can be ignored. (Can't use the "no premature optimization", as when this function is used, it REALLY slows everything down.) Comment #11 Error-reporting system is crude, requiring manual use of __FILE__ and of __LINE__, something less inelegant could surely be devised. Mitigation/Rebuttal An elegant error-reporting system would most likely dwarf this little program, and using someone else's would incur a needless dependency. Comment #12 Board initialization is dumb. It assigns null values that get overwritten anyway. Mitigation/Rebuttal That's the leftover changes that resulted from tracking down a data-corrupting bug. Comment #13 No unit tests. Mitigation/Rebuttal Scratch code, done for fun, all in one file. Not a lot of externally verifiable behavior aside from computing actual solutions. Comment #14 Harcoded to this particular problem. Board size need not be fixed, nor does the board need to be square. The generalization to a board of m x n is fairly basic and more useful. Mitigation/Rebuttal Scratch code, first attempt. Generalizing the algorithm to handle arbitrarily-sized problems is a useful task for subsequent versions. Comment #15 Shouldn't have to specify the move-slot. Mitigation/Rebuttal Yes, but it was easier to do so at the time. Comment #16 Memory leaks. Mitigation/Rebuttal Scratch program, etc. The outstanding queue that is "just left in place" will get cleaned up when at program exit. The big problem is the partial branches (non-solution leaves), but it works regardless. This is where GC would be handy. Comment #17 Only first solution found. Mitigation/Rebuttal That was all that was being looked for. It satisfies the original requirement. Comment #18 Input checking is minimal, and doesn't verify that there is indeed a blank, or that the "final" string is a permutation of the "start" string, which would send the algorithm off to never-never land. Mitigation/Rebuttal A missing blank in the start string should result in program exit when the blank is searched for; an impossible solution due to mismatched strings will not result in an infinite loop, as the search tree is self limiting when we eliminate loops. (Despite the comment claiming it is only an optimization step.) Comment #19 Return codes are only success or failure, not something more useful. Mitigation/Rebuttal This program doesn't really do anything useful that checking the return codes would be useful _for_. Error conditions emit output to stderr before exiting with a failure takes place. Comment #20 Code isn't very tight. Lots of extraneous statements, needless variable assignments, etc. Mitigation/Rebuttal Yup. Scratch code, etc... [] ----------------------------------------------------------------------------- -- Sure, the code is ugly, fragile, and crude It's basically a throwaway solution, dude. Stewart Stremler -- [email protected] http://www.kernel-panic.org/cgi-bin/mailman/listinfo/kplug-lpsg
