On Thu, Mar 13, 2014 at 2:34 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
>
>> Shouldn't this logic [to decide what the printf arguments should
>> be] also be encoded in the table?
>> ...
>> The same argument also applies to computation of the 'name' variable
>> above. It too can be pushed into the the table.
>
> Because the "printf argument" logic does not have to be in the
> table, the same argument does not apply to the 'name' thing.
>
> After looking at the v5 patch, I do not think an extra two-element
> array to switch between remote vs shortname is making it any easier
> to read. I would have to say that personally I find that
>
> const char *name[] = {remote, shortname};
> ... long swath of code ...
> printf_ln(... name[!remote_is_branch] ...);
>
> is a lot harder to read than:
>
> printf_ln(... remote_is_branch ? shortname : branch ...);
Indeed, that's a step backward, and is not what was asked. Merely
pushing data into tables does not make the logic table-driven
(emphasis on *driven*). The GSoC microproject did not demand a
table-driven approach, but instead asked students if such an approach
would make sense. A more table-driven approach might look something
like this:
struct M { const char *s; const char **a1; const char **a2; }
message[][2][2] = {{{
{ "Branch %s set ... %s ... %s", &shortname, &origin },
...
}},{{
{ "Branch %s set ... %s", &remote, NULL },
...
}}};
const struct M *m = message[!remote_is_branch][!origin][!rebasing];
printf_ln(m->s, local, *m->a1, *m->a2);
Whether this approach is more clear than the original code is a matter
for debate [1], however, it's obvious from the table which arguments
belong with each message, and the printf_ln() invocation does not
require any logic. When moving only messages into a table, they become
disconnected from their arguments which makes reasoning about them a
bit more difficult. The original code does not have this problem, nor
does a table-driven approach.
[1]: While ungainly, the original code may not be sufficiently bad to
warrant the extra complications of a table. A simple refactoring, such
as [2], can make the code a bit easier to read without adding
complexity.
[2]: http://thread.gmane.org/gmane.comp.version-control.git/243704
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html