Re: [PATCH v7 4/5] dir_iterator: refactor state machine model

From: Daniel Ferreira (theiostream) <bnmvco@gmail.com>
Date: Apr 3, 2017, 18:03 UTC
Message-ID: <CAEA2_R+EBbrD1rbcrP598AKJAEZDZfGY-ED8g+ehgAaL9tLG8A@mail.gmail.com>
In-Reply-To: <76c92397-8e15-b2fb-432f-0ae3d987ffa8@alum.mit.edu>

On Mon, Apr 3, 2017 at 12:36 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:

Show 5 quoted lines

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