On Thu, Jan 26, 2017 at 04:18:06PM +0700, Duy Nguyen wrote:
> > Hmm. I see what you're trying to do here, and abstract the repeated
> > bits. But I'm not sure the line-count reflects a real simplification.
> > Everything ends up converted to an enum, and then that enum just expands
> > to similar C code.
>
> It's not simplification, but hopefully for better maintainability. This
>
> if (strcmp(arg, "--remotes")) {
> if (preceded_by_exclide())
> does_something();
> else if (preceded_by_decorate())
> does_another()
> } else if (strcmp(arg, "--branches")) {
> if (preceded_by_exclide())
> does_something();
> else if (preceded_by_decorate())
> does_another()
> }
>
> starts to look ugly especially when the third "preceded_by_" comes
> into picture. Putting all "does_something" in one group and
> "does_another" in another, I think, gives us a better view how ref
> selection is handled for a specific operation like --exclude or
> --decorate-ref.
I agree that would be ugly. But the current structure, which is:
if (strcmp(arg, "--remotes")) {
handle_refs(...);
cleanup();
} else if(...) {
handle_refs(...);
cleanup();
}
does not seem so bad, and pushes those conditionals into the
handle_refs() function, where they only need to be expressed once (I
didn't look, but I wonder if you could push the cleanup steps in there,
too, or if there is a caller who wants to handle() multiple times before
cleaning up).
-Peff
