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

From: Dragos Foianu <dragos.foianu@gmail.com>
Date: Mar 17, 2014, 11:46 UTC
Message-ID: <loom.20140317T120153-546@post.gmane.org>
In-Reply-To: <CAPig+cTodcfSVmHZeHuAj2kuE_CxuZqZuaNHv33hrhDmQuSmuA@mail.gmail.com>

Eric Sunshine <sunshine <at> sunshineco.com> writes:

Show 13 quoted lines

> In fact, this change is not table-driven (emphasis on *driven*). It
> merely moves the strings into a table, but all the logic is still in
> the code. To be table-driven, the logic would be encoded in the table
> as well, and that logic would *drive* the code.
> 
> This is not to say that the code must be table-driven. The GSoC
> microproject merely asks if doing so would make sense. Whether it
> would is for you to decide, and then to explain to reviewers the
> reason(s) why you did or did not make it so.
>
> The if-chain is just sufficiently complex that the print messages may
> actually help the reader understand the logic of the code, so this
> argument seems specious.

I understand now after reading your review. I will try to fix this in a
future attempt.

> 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.

Show 5 quoted lines

> 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.

I have seen N_() used in other code but I wasn't sure what its purpose was.

Thank you very much for the review.