Re: [PATCH] branch.c: simplify chain of if statements

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Mar 17, 2014 at 7:46 AM, Dragos Foianu <dragos.foianu@xxxxxxxxx> wrote:
> Eric Sunshine <sunshine <at> sunshineco.com> writes:
>> Matthieu already mentioned [2] that this sort of "lego" string
>> construction is not internationalization-friendly. See section 4.3 [3]
>> of the gettext manual for details.
>
> I was hoping to get away with using less memory by having only four entries
> in the table. I suppose that is not possible. The rebasing check can still
> be moved outside of the four if statements and calculate the index
> correctly. The strings would then have to be arranged in such a way to make
> this work.
>
> Using a multiple-dimension array as suggested in other submissions for this
> particular microproject would probably be better, but it has already been done.

If a multi-dimension table is indeed better than other alternatives,
then that's a good reason to choose it, even if others have already
used that approach in their submissions. It's more important that the
code is clean and easy to understand and maintain than to be clever.

If you're really interested in trying an approach not already
submitted by others, take a look at Jonathan's idea [1]. If you play
around with it and find that it actually does make the code clearer
and simpler, then perhaps it's worth submitting. If not, then not.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/198882/focus=198902

>> These hard-coded indexing constants (0, 1, 2, 3) are fragile and
>> convey little meaning to the reader. Try to consider how to compute
>> the index into verbose_prints[] based upon the values of
>> 'remote_is_branch' and 'origin'. What are the different ways you could
>> do so?
>
> I was going to do something like this: if !remote_is_branch the index goes
> incremented by 2, because the first two entries are of no interest and if
> !origin, the index is incremented by 1. This would correctly compute the
> index. It should also work with the rebasing check if the four
> rebasing-specific messages are at the end of the table and when rebasing the
> index is set to start at those messages.
>
> The reason I did not go with this is because I would still need the four ifs
> in order to keep the bug check part of the code. I might be able to find a
> work-around for it on the second attempt.

Since the result is just a number, its possible to compute it directly
without conditionals, however, it does start resembling a magical
incantation. (I'll comment further in your v2 submission.)

> I have seen N_() used in other code but I wasn't sure what its purpose was.
>
> Thank you very much for the review.
--
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




[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]