On Mon, Apr 3, 2017 at 12:36 AM, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > As far as I can tell, you got the logic in this complicated big loop > correct on the first try (well, if we ignore v6 :-) ), even as you added > new features. I think that's good evidence that the new structure is > more comprehensible than the old, plus the new tests probably helped. > That's a big win! Thanks for ignoring v6 ;) Another gain is that the other proposed features (only iterate over directories, do not recurse into subdirectories) are also quite easy to add with this new structure. > It's not ideal that the test code depends on the numerical values of the > flag constants, and it makes the tests harder to understand. It would be > better if this program were to accept options like `--pre-order`, > `--post-order`, etc., as I suggested in an earlier round of review. Although it does make tests harder to understand, if we were to specify how to iterate with human-readable flags we'd add the getopt() + flag configuration overhead to this helper program to be able to handle all cases properly. Additionally, new flags added to dir_iterator would have to edit the test program as well, generating extra work. I personally think that the string describing the test in the script is enough to explain what the flag-as-argument is doing for the sake of readability. The only gain I see in your suggestion is that we avoid the programmer committing errors calculating the flag by hand when writing the test. That said, I'd appreciate some more thought on this. > Michael > Thanks for the review. I agree with all other points and I'll address them in a next series.