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