Re: [PATCHv2] branch.c: simplify chain of if statements
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 18, 2014, 22:31 UTC
- Message-ID
- <CAPig+cS9QApn1T3-R8n+W+1ee9FbNftsmhrr90SJKs+gqzvC5A@mail.gmail.com>
- In-Reply-To
- <1395071493-31435-1-git-send-email-dragos.foianu@gmail.com>
Thanks for the resubmission. Comments below...
On Mon, Mar 17, 2014 at 11:51 AM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
Show 5 quoted lines
> This patch uses a table to store the different messages that can > be emitted by the verbose install_branch_config function. It > computes an index based on the three flags and prints the message > located at the specific index in the table of messages. If the > index somehow is not within the table, we have a bug.
Most of this text can be dropped due to redundancy.
Saying "This patch..." is unnecessary.
The remaining text primarily says in prose what the patch itself conveys more concisely and precisely. It's easier to read and understand the actual code than it is to wade through a lengthy description of the code change.
Speak in imperative voice: "Use a table to store..."
You might, for instance, say instead something like this:
install_branch_config() uses a long, somewhat complex if-chain to
select a message to display in verbose mode. Simplify the logic
by moving the messages to a table from which they can be
easily retrieved without complex logic.Show 38 quoted lines
> Signed-off-by: Dragos Foianu <dragos.foianu@gmail.com>
> ---
> branch.c | 44 +++++++++++++++++++++++++-------------------
> 1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..95645d5 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -54,6 +54,18 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
> struct strbuf key = STRBUF_INIT;
> int rebasing = should_setup_rebase(origin);
>
> + const char *messages[] = {
> + N_("Branch %s set up to track local ref %s."),
> + N_("Branch %s set up to track remote ref %s."),
> + N_("Branch %s set up to track local branch %s."),
> + N_("Branch %s set up to track remote branch %s from %s."),
> + N_("Branch %s set up to track local ref %s by rebasing."),
> + N_("Branch %s set up to track remote ref %s by rebasing."),
> + N_("Branch %s set up to track local branch %s by rebasing."),
> + N_("Branch %s set up to track remote branch %s from %s by rebasing.")
> + };
> + int index = 0;
> +
> if (remote_is_branch
> && !strcmp(local, shortname)
> && !origin) {
> @@ -76,28 +88,22 @@ void install_branch_config(int flag, const char *local, const char *origin, cons
> }
> strbuf_release(&key);
>
> + if (origin)
> + index += 1;
> + if (remote_is_branch)
> + index += 2;
> + if (rebasing)
> + index += 4;Other submissions have computed this value mathematically without need for conditionals. For instance, we've seen:
index = (!!origin << 0) + (!!remote_is_branch << 1) + (!!rebasing << 2)
as, well as the equivalent:
index = !!origin + !!remote_is_branch * 2 + !!rebasing * 4
Although this works, it does place greater cognitive demands on the reader by requiring more effort to figure out what is going on and how it relates to table position. The original (ungainly) chain of 'if' statements in the original code does not suffer this problem. It likewise is harder to understand than merely indexing into a multi-dimension table where each variable is a key.
Show 26 quoted lines
> if (flag & BRANCH_CONFIG_VERBOSE) {
> if (remote_is_branch && origin)
> - printf_ln(rebasing ?
> - _("Branch %s set up to track remote branch %s from %s by rebasing.") :
> - _("Branch %s set up to track remote branch %s from %s."),
> - local, shortname, origin);
> - else if (remote_is_branch && !origin)
> - printf_ln(rebasing ?
> - _("Branch %s set up to track local branch %s by rebasing.") :
> - _("Branch %s set up to track local branch %s."),
> - local, shortname);
> - else if (!remote_is_branch && origin)
> - printf_ln(rebasing ?
> - _("Branch %s set up to track remote ref %s by rebasing.") :
> - _("Branch %s set up to track remote ref %s."),
> - local, remote);
> - else if (!remote_is_branch && !origin)
> - printf_ln(rebasing ?
> - _("Branch %s set up to track local ref %s by rebasing.") :
> - _("Branch %s set up to track local ref %s."),
> - local, remote);
> + printf_ln(_(messages[index]),
> + local, shortname, origin);
> else
> + printf_ln(_(messages[index]),
> + local, (!remote_is_branch) ? remote : shortname);It's possible to simplify this logic and have only a single printf_ln() invocation. Hint: It's safe to pass in more arguments than there are %s directives in the format string.
> +
> + if (index < 0 || index > sizeof(messages) / sizeof(*messages))
> die("BUG: impossible combination of %d and %p",
> remote_is_branch, origin);You can use ARRAY_SIZE() in place of sizeof(...)/sizeof(...).
Since an out-of-bound index would be a programmer bug, it would probably be more appropriate to use an assert(), just after 'index' is computed, rather than if+die(). The original code used die() because it couldn't detect the error until the end of the if-chain.
> } > -- > 1.8.3.2