Re: [PATCH 12/13] remote.c: refactor get_remote_ref_states()

From: Jay Soffian <jaysoffian@gmail.com>
Date: Feb 23, 2009, 07:55 UTC
Message-ID: <76718490902222355v7510d2f9p51ade61d1f257146@mail.gmail.com>
In-Reply-To: <20090223065048.GC24517@coredump.intra.peff.net>

On Mon, Feb 23, 2009 at 1:50 AM, Jeff King <peff@peff.net> wrote:

Show 17 quoted lines

> On Mon, Feb 23, 2009 at 01:29:00AM -0500, Jay Soffian wrote:
>
>> get_remote_ref_states() has three callers, but each is interested in
>> slightly different information. Give it a bit-field flag so that callers
>> can specify which pieces of information they need.
>
> Hmph. I think this is probably an indication that
> get_remote_ref_states() should really be 3 functions:
>
>  common_stuff();
>  query_one();
>  query_two();
>
> and then callers can choose the subset they are interested in.
>
> Which is really more or less equivalent; it just seems like extra
> obfuscation to have a single function with a bit-field.

I see your hmph and raise you a hmph. :-)

Well, I _had_ tried as you suggested first, and thought it yuckier. It would actually be more like:

caller1() {
  setup_for_get();
  get_thing_one();
}

caller2() {
  setup_for_get();
  get_thing_two();
}

caller3() {
  if (query) {
     setup_for_get();
     get_thing_one();
     get_thing_two();
     get_thing_three();
} else {
     get_thing_one_noquery()
     get_thing_two_noquery();
}

As opposed to:

caller1() {
  get_things(ONE)
}

caller2() {
  get_things(TWO)
}

caller3() {
  things = 0;
  if (query)
     things = (ONE|TWO|THREE)
  get_things(things)
}

I'm not sure why passing a flag saying what you want is obfuscating. Also, I did find other places in git that do this sort of thing (e.g., get_remote_heads).

(Awaiting Junio's lucid reply explaining why my justification is bogus.) :-)

j.