Re: [PATCH 2/5] revision.c: group ref selection options together

From: Jeff King <peff@peff.net>
Date: Jan 26, 2017, 14:19 UTC
Message-ID: <20170126141957.qcc75szbimqove2n@sigill.intra.peff.net>
In-Reply-To: <CACsJy8CeUWD9ux3Q-VRhwR4-qbSD69CzH2mmztVH_-cx=31h8w@mail.gmail.com>

On Thu, Jan 26, 2017 at 04:18:06PM +0700, Duy Nguyen wrote:

Show 24 quoted lines

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