threads / discuss / 36198

[PATCHv2] branch.c: simplify chain of if statements

Subject: [PATCHv2] branch.c: simplify chain of if statements

## tl;dr

7 messages between Mar 17, 2014 and Mar 21, 2014.

replies: 6people: 3as markdown or json

Dragos Foianu· Mar 17, 2014, 15:51 UTC · lore

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.

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;
+
 	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);
+
+		if (index < 0 || index > sizeof(messages) / sizeof(*messages))
 			die("BUG: impossible combination of %d and %p",
 			    remote_is_branch, origin);
 	}

-- 
1.8.3.2
Eric Sunshine· Mar 18, 2014, 22:31 UTC · re: Dragos Foianu · lore

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

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
Eric Sunshine· Mar 18, 2014, 23:50 UTC · re: Eric Sunshine · lore

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

On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 28 quoted lines
> On Mon, Mar 17, 2014 at 11:51 AM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
>> 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.
>>
>> 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
>> +       int index = 0;
>> +       if (origin)
>> +               index += 1;
>> +       if (remote_is_branch)
>> +               index += 2;
>> +       if (rebasing)
>> +               index += 4;
>> +
>> +               if (index < 0 || index > sizeof(messages) / sizeof(*messages))
>>                         die("BUG: impossible combination of %d and %p",
>>                             remote_is_branch, origin);

One other observation: You have a one-off error in your out-of-bounds check. It should be 'index >= sizeof...'

Show 10 quoted lines
> 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
Dragos Foianu· Mar 19, 2014, 23:12 UTC · re: Eric Sunshine · lore

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

Eric Sunshine <sunshine <at> sunshineco.com> writes:
Show 16 quoted lines
> 
> 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.

I have seen other submissions using this logic, but I didn't think it accomplished the goal of the patch - simplifying the code. Not that my approach does either, but I found it a little easier to understand.

Indexing into a table like this is always going to have this problem so this is probably not the right approach to accomplishing the microproject's goals.

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

Indeed. It's a habit of mine to pass the exact number of arguments to printf functions and I can't seem to get away from it.

Show 6 quoted lines
> 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.

Thank you for this hint. Using already defined helpers in the project is better and will prevent the need to patch the constructs later on.

> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine <at>
sunshineco.com> wrote:
> 
> One other observation: You have a one-off error in your out-of-bounds
> check. It should be 'index >= sizeof...'
Well this is embarrasing.

Thank you again for the feedback. It's incredibily helpful and I learned a lot from submitting these patches. Making the code simple is harder than it appears at first sight.

I'm not sure it's worth pursuing the table approach further, especially since a solution has already been accepted and merged into the codebase.

In this case, is it okay to try another microproject? I was thinking about trying #17 (finding bugs/inefficiencies in builtin/apply.c), but I've already had my one microproject.

All the best, Dragos

Eric Sunshine· Mar 21, 2014, 00:40 UTC · re: Dragos Foianu · lore

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

On Wed, Mar 19, 2014 at 7:12 PM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
Show 8 quoted lines
> Eric Sunshine <sunshine <at> sunshineco.com> writes:
>> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine <at>
> sunshineco.com> wrote:
>>
>> One other observation: You have a one-off error in your out-of-bounds
>> check. It should be 'index >= sizeof...'
>
> Well this is embarrassing.

It's a good illustration of the value of the review process. It's easy to overlook omissions and problems in our one's work because one reads it with the bias of knowing what it's _supposed_ to say. Reviewers (hopefully) don't have such bias: they read the code afresh.

Show 6 quoted lines
> Thank you again for the feedback. It's incredibily helpful and I learned a
> lot from submitting these patches. Making the code simple is harder than it
> appears at first sight.
>
> I'm not sure it's worth pursuing the table approach further, especially
> since a solution has already been accepted and merged into the codebase.
Agreed.
> In this case, is it okay to try another microproject? I was thinking about
> trying #17 (finding bugs/inefficiencies in builtin/apply.c), but I've
> already had my one micro project.
According to the description for #17, there are plenty of opportunities, so...
> All the best,
> Dragos
Eric Sunshine· Mar 21, 2014, 00:44 UTC · re: Eric Sunshine · lore

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

On Thu, Mar 20, 2014 at 8:40 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 14 quoted lines
> On Wed, Mar 19, 2014 at 7:12 PM, Dragos Foianu <dragos.foianu@gmail.com> wrote:
>> Eric Sunshine <sunshine <at> sunshineco.com> writes:
>>> On Tue, Mar 18, 2014 at 6:31 PM, Eric Sunshine <sunshine <at>
>> sunshineco.com> wrote:
>>>
>>> One other observation: You have a one-off error in your out-of-bounds
>>> check. It should be 'index >= sizeof...'
>>
>> Well this is embarrassing.
>
> It's a good illustration of the value of the review process. It's easy
> to overlook omissions and problems in our one's work because one reads
> it with the bias of knowing what it's _supposed_ to say. Reviewers
> (hopefully) don't have such bias: they read the code afresh.

And, this is a perfect example. I knew that I wanted to say "problems in one's own work", and even though I proof-read, I still missed that I wrote "problems in our one's work".

Junio C Hamano· Mar 21, 2014, 16:50 UTC · re: Dragos Foianu · lore

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

Dragos Foianu <dragos.foianu@gmail.com> writes:
> I'm not sure it's worth pursuing the table approach further, especially
> since a solution has already been accepted and merged into the codebase.
Yes.

I would further say that you already qualify as having finished a microproject, if I were a part of the candidate selection panel.

The important thing is for potential candidates to learn the process, not to have their change merged somewhere my tree, and you and many others who did a microproject and tasted the process of proposing a change, getting reviewed and learning what are expected of their patch submissions have finished that part already.

← back to recent threads