Charvi Mendiratta <charvi077@xxxxxxxxx> writes:
> On Sun, 14 Mar 2021 at 07:55, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>>
>> Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes:
>>
>> > The one thing that does bother me, however, is the name of the
>> > function, get_alpha_len(), which tells you (somewhat) literally what
>> > it does but doesn't convey to the reader its actual purpose (which is
>> > something we should strive for when naming functions and variables).
>>
>> I actually think the helper function that is used as a building
>> block the "subcommand parser" uses should be named more directly
>> to represent what it does (i.e. look for a run of alphas) than
>> what it means (i.e. look for a run of letters allowed in a
>> subcommand name). IOW
>>
>> char *end = skip_alphas(ptr);
>> if (*end == ':' && ptr != end) {
>> /*
>> * ptr..end could be a subcommand in
>> * "--fixup=<subcommand>:"; see if it is a known one
>> */
>> *end = '\0';
>> if (!strcmp(ptr, "amend"))
>> ... do the amend thing ...
>> else if (!strcmp(ptr, "reword"))
>> ... do the reword thing ...
>> else
>> ... we do not know such a subcommand yet ...
>> } else {
>> /* assume it is --fixup=<command> form */
>> ...
>> }
>>
>> conveys more information to readers than a variant where you replace
>> "skip_alphas" with "skip_subcommand_chars" without losing any
>> information.
>>
>
> I thought to just rename get_alpha_len() to skip_alpha() that returns
> alpha length. But even removing the "len" variable and implementing as
> suggested above seems a better and clear alternative. I also agree to
> update it.
>
> Thanks for the suggestions.
FWIW I am also fine with Eric's simpler "open code it right there"
suggestion in this case. Just like the "skip alphas" suggestion, it
makes the logic to parse subcommand name out isolated to a single
place without asking readers to refer to the implementation of a
helper, and it would be short enough.
Thanks.
