music-function.cc: code simplification; issue 4421

music-function.cc: code simplification; issue 4421

[Keith, 2015-06-21 04:12:34]

cleaner loop advancement

[dak, 2015-06-21 15:45:33]

Code structure/readability looks like a definite improvement.  While I feel I
might be overlooking something, the code does not seem to leave much of an
opportunity for doing so.

Can you create a Google Code issue so that Patchy may have a go at it?

https://codereview.appspot.com/247190043/diff/20001/lily/music-function.cc
File lily/music-function.cc (right):

https://codereview.appspot.com/247190043/diff/20001/lily/music-function.cc#ne...
lily/music-function.cc:113: signature = scm_cdr (signature);
I prefer continue; here, and then using just if (optional) afterwards.  That
saves the reader from having to follow all of the ... else if ... else ... chain
just to find out that nothing more happens in this loop.

https://codereview.appspot.com/247190043/diff/20001/lily/music-function.cc#ne...
lily/music-function.cc:129: && scm_is_pair (scm_car (signature)));
The case for continue; here for continuing the main loop is less clear cut.  I'd
probably use it anyway even if it means that all of the actual looping is routed
through continue; since the final case is the error case.

[Keith, 2015-06-21 18:34:43]

On Sun, 21 Jun 2015 08:45:33 -0700, <dak@gnu.org> wrote:

> lily/music-function.cc:113: signature = scm_cdr (signature);
> I prefer continue; here, and then using just if (optional) afterwards.
> That saves the reader from having to follow all of the ... else if ...
> else ... chain just to find out that nothing more happens in this loop.
>

You know, though, that other people read code by looking at the structure
first.
These people, whom I like, already have the if-else chain in mind before reading
deeper.

while (there are more arguments) {
   setup()
   if (argument matches)
     ...
   else if (argument was optional)
     ...
   else
     error()
}

These people, whom I like, would be annoyed to see

while (there are more arguments) {
   setup()
   if (argument matches)
     ...
   if (argument was optional)
     ...
   else
      error()
}

because we think at first the later if-else is executed each time through the
loop. We are also likely to put a debug print at the close of a loop to
understand the state on completion.

I tend to see 'continue' used only to skip the body in unusual cases, and
usually near the top of the loop.

[dak, 2015-06-21 22:17:31]

"Keith OHara" <k-ohara5a5a@oco.net> writes:

> On Sun, 21 Jun 2015 08:45:33 -0700, <dak@gnu.org> wrote:
>
>> lily/music-function.cc:113: signature = scm_cdr (signature);
>> I prefer continue; here, and then using just if (optional) afterwards.
>> That saves the reader from having to follow all of the ... else if ...
>> else ... chain just to find out that nothing more happens in this loop.
>>
>
> You know, though, that other people read code by looking at the structure
first.
> These people, whom I like, already have the if-else chain in mind before
reading deeper.
>
> while (there are more arguments) {
>   setup()
>   if (argument matches)
>     ...
>   else if (argument was optional)
>     ...
>   else
>     error()
> }

You cannot have "the if-else chain in mind before reading deeper" if it
does not even fit on screen.  And those hypothetical people are either
interested in the case where the argument matches in which case it makes
little sense to skip reading it in order to see what happens last in the
loop first, or they aren't interested in the case where the argument
matches in which case it makes no difference to them whether or not
there is a continue; in the branch that isn't taken.

Let me quote from a well-known English writer
<URL:https://en.wikisource.org/wiki/A_Tramp_Abroad/Appendix_D>:

    We have the Parenthesis disease in our literature, too; and one may
    see cases of it every day in our books and newspapers: but with us
    it is the mark and sign of an unpractised writer or a cloudy
    intellect, whereas with the Germans it is doubtless the mark and
    sign of a practised pen and of the presence of that sort of luminous
    intellectual fog which stands for clearness among these people. For
    surely it is not clearness,\u2014it necessarily can't be
    clearness. Even a jury would have penetration enough to discover
    that. A writer's ideas must be a good deal confused, a good deal out
    of line and sequence, when he starts out to say that a man met a
    counsellor's wife in the street, and then right in the midst of this
    so simple undertaking halts these approaching people and makes them
    stand still until he jots down an inventory of the woman's
    dress. That is manifestly absurd. It reminds a person of those
    dentists who secure your instant and breathless interest in a tooth
    by taking a grip on it with the forceps, and then stand there and
    drawl through a tedious anecdote before they give the dreaded
    jerk. Parentheses in literature and dentistry are in bad taste.

So I don't see how you gain readability by changing the amount of lines
you need to scan for understanding loop behavior when the argument
matches from 21 lines (which fit on a screen even on an 80x24 terminal)
to 47 lines, leafing back and forth.

> These people, whom I like, would be annoyed to see
>
> while (there are more arguments) {
>   setup()
>   if (argument matches)
>     ...
>   if (argument was optional)
>     ...
>   else
>      error()
> }

I probably have more German friends than you do but I still prefer being
able to cross off a topic rather than drag as much unfinished business
along as long as I can.  It's not fun when either debugging stuff in
your head or actually single-stepping through it.

> because we think at first the later if-else is executed each time
> through the loop.  We are also likely to put a debug print at the
> close of a loop to understand the state on completion.

C++ has more than one way to leave a block.  There are returns,
exceptions, breaks, continues.  Put the debug print where you want to
see information regarding a particular case rather than elsewhere while
hoping that the code will come around at some point of time.

> I tend to see 'continue' used only to skip the body in unusual cases,
> and usually near the top of the loop.

This is near the top of the loop.  It's not an "unusual case" but rather
the most frequent one but I don't see why that makes it a better rather
than a worse idea to have to read through 47 instead of 21 lines in
order to figure out what happens.

-- 
David Kastrup

[dak, 2015-06-22 07:09:21]

Stylistic differences not covered by our coding conventions don't preclude a
change going on countdown and getting committed: the one doing the actual work
is the ultimate arbiter for that.  But it still needs a Google Code issue.